Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert let and const in maplibre to enable es5 test #7077

Open
archmoj opened this issue Aug 8, 2024 · 12 comments
Open

Convert let and const in maplibre to enable es5 test #7077

archmoj opened this issue Aug 8, 2024 · 12 comments
Assignees
Labels
feature something new P2 needed for current cycle

Comments

@archmoj
Copy link
Contributor

archmoj commented Aug 8, 2024

@birkskyum if you recall, we had to disable the es5 tests in #7060.
I was wondering if you could simply try replacing some const and let to var in the maplibre bundle (or give webpack config another try #7015 (comment)) and investigate if we could enable es5 test for a bit longer until switching to es6 in #6909?
Thank you!

Screenshot from 2024-08-08 12-53-02

@birkskyum
Copy link
Contributor

I've tried a few things, including making an es5 build of the unminified production build of maplibre, and loading it in / rebuilding, but for the no-new-func, i still see these:

Screenshot 2024-08-08 at 19 49 53

The no-es6-dist appear to pass though - what error do you see there?

@archmoj
Copy link
Contributor Author

archmoj commented Aug 8, 2024

@birkskyum please note that it is actually the second test catches these (not the es6 test).
Also to test it locally you needed to build the bundles in dist folder.
So what the error you displayed in the image below is the one we need to possibly fix.

@birkskyum
Copy link
Contributor

Okay, so the first test can be re-enabled already. For the no-new-func, I'm not sure which const and let assignments it's flagging.

@birkskyum
Copy link
Contributor

is there a plotly-map similar to the plotly-mapbox that we can use to narrow down the issue and make new builds faster?

@archmoj
Copy link
Contributor Author

archmoj commented Aug 8, 2024

It's coming from

let supportsOffscreenCanvas;
function offscreenCanvasSupported() {
    if (supportsOffscreenCanvas == null) {
        supportsOffscreenCanvas = typeof OffscreenCanvas !== 'undefined' &&
            new OffscreenCanvas(1, 1).getContext('2d') &&
            typeof createImageBitmap === 'function';
    }
    return supportsOffscreenCanvas;
}

@birkskyum
Copy link
Contributor

birkskyum commented Aug 8, 2024

The errors I get mention a use of const. I tried loading in a es5 build of maplibre where that specific section looks like this:

var supportsOffscreenCanvas;
function offscreenCanvasSupported() {
    if (supportsOffscreenCanvas == null) {
        supportsOffscreenCanvas = typeof OffscreenCanvas !== 'undefined' &&
            new OffscreenCanvas(1, 1).getContext('2d') &&
            typeof createImageBitmap === 'function';
    }
    return supportsOffscreenCanvas;
}

and i still got the errors after a fresh rebuild

@archmoj
Copy link
Contributor Author

archmoj commented Aug 8, 2024

Until we publish plotly.js-map-dist packages similar to https://www.npmjs.com/package/plotly.js-mapbox-dist,
to create a custom bundle including maplibre traces, one could run

npm run custom-bundle -- --unminified --traces choroplethmap,densitymap,scattermap

But strangely I don't get an error on this custom bundle!
I started wondering this might be a webpack problem?!

@archmoj
Copy link
Contributor Author

archmoj commented Aug 8, 2024

Oops. It actually did throw an error with the modified script for custom map bundle.
So it could be something you may fix at maplibre level.

@archmoj
Copy link
Contributor Author

archmoj commented Aug 8, 2024

Here is a faster command for testing:

npm run custom-bundle -- --unminified --traces scattermap && node ./node_modules/eslint/bin/eslint.js --no-ignore --no-eslintrc --no-inline-config --rule '{no-new-func: warn}' dist/plotly-custom.js

@birkskyum
Copy link
Contributor

birkskyum commented Aug 8, 2024

I've tried running this, and it turns out it's maplibre's dependencies that use various es6 (es2015) features, like glob which uses #private, and it makes it a bit challenging to get the es5 compilation running.

@birkskyum
Copy link
Contributor

birkskyum commented Aug 8, 2024

@archmoj , I'm don't think I can resolve this readily, since es6 is used in a lot of places at this point, because library authors take it for granted due to pages like this - it might be worth checking in the internal tools if there are plotly users without es6 support, and how many.

@archmoj
Copy link
Contributor Author

archmoj commented Aug 8, 2024

Thanks @birkskyum for the note.
I think the fix should be in plotly.js/webpack.config.js not on the maplibre.
We already have es6 dependencies similar to maplibre.

@gvwilson gvwilson added feature something new P2 needed for current cycle labels Aug 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new P2 needed for current cycle
Projects
None yet
Development

No branches or pull requests

3 participants