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

ECMAScript Modules #1595

Open
birkskyum opened this issue Aug 31, 2022 · 17 comments
Open

ECMAScript Modules #1595

birkskyum opened this issue Aug 31, 2022 · 17 comments
Labels
enhancement New feature or request PR is more than welcomed Extra attention is needed

Comments

@birkskyum
Copy link
Member

birkskyum commented Aug 31, 2022

ESM is the new standard with import/export etc. We don't use it now because there are some old mapbox dependencies that only export .cjs, but nothing more than we can handle. Also, firefox has for 5 years struggled to support this in web workers, and now it's about time to move on:

Firefox (Update: Resolved from v114)

Web worker support ships with Firefox 114 PR, Release notes

The issue was around lack ESM support for web workers: https://caniuse.com/mdn-api_worker_worker_ecmascript_modules
5+ years of ongoing effort: https://bugzilla.mozilla.org/show_bug.cgi?id=1247687

Benchmarks

I once attempted to move the benchmark to ESM #964 , but found the change wasn't backward compatible with the already uploaded benchmarks of prev versions, so it was rolled back #969 .

Point of this issue

This can be a tracking issue where we first map the dependencies we have that aren't compatible, and then keep this updated when they are updated

@birkskyum birkskyum changed the title ECMAScript ECMAScript Modules Aug 31, 2022
@HarelM
Copy link
Collaborator

HarelM commented Aug 31, 2022

Do you mean that we currently export maplibre as commonjs and we should export it as esm?
I think the problem of switching out of commonjs besides the FF support was the ability to have the worker URL pushed while building this.
There are some complicated build steps to facilitate for this which I think are very hard to split.
I've looked at this when I tried to understand if we could leverage tree shaking here: #977
I generally think this is a good idea, even better if we can introduce it as part of 3.0 version as a breaking change.
But I'm not sure how to solve all the build complexity with the workers url and also the shared code between the main thread and the worker thread...

@birkskyum
Copy link
Member Author

birkskyum commented Aug 31, 2022

Yes, export maplibre as esm. Sure, I think we might be able to take small steps towards it. A major blocker previously was i.e. also all this node assert that would break the dev build if exported as esm. The first step is likely to have the build as esm.

@backspaces
Copy link

backspaces commented Sep 4, 2022

The published dist/ dir already has multiple versions: https://unpkg.com/browse/[email protected]/dist/
And indeed, the individual src files appear use import/export, although "bare".

So shouldn't it be easy to just add a new one: maplibre-gl-esm.js?
And add a "module": "dist/maplibre-gl-esm.js" to package.json?
That seems to be the common approach for folks wanting to support both forms.

For now, skypack works:

        import maplibreGl from 'https://cdn.skypack.dev/maplibre-gl'

.. but we'd like to avoid skypack's conversions which sometimes fail. And it would be nice to use other cdn's as well.

@geraldo
Copy link
Contributor

geraldo commented Sep 19, 2022

import maplibreGl from 'https://cdn.skypack.dev/maplibre-gl'

@backspaces That doesn't work for me:

var map = new maplibreGl.Map()

gives me an [ERR_UNSUPPORTED_ESM_URL_SCHEME] error.

@backspaces
Copy link

Try it using this:

<!DOCTYPE html>
<html>
<head>
    <title>Imports</title>
</head>
<body>
    <script type="module">
        import maplibregl from 'https://cdn.skypack.dev/maplibre-gl'
        console.log('https://cdn.skypack.dev/maplibre-gl', maplibregl)
    </script>
</body>
</html>

@geraldo
Copy link
Contributor

geraldo commented Sep 21, 2022

Thanks, that works fine when run directly in the browser, and even shows a basic map:

var map = new maplibregl.Map({
	container: 'map',
	style: 'https://demotiles.maplibre.org/style.json',
	center: [0, 0],
	zoom: 0
})

But when I try to run it with node (using this https module loader), new maplibregl.Map does throw an error:

ReferenceError: devicePixelRatio is not defined

@birkskyum
Copy link
Member Author

The devicePixelRatio likely fail because there is no device/display. What do you hope to achieve running it in node ? Depending on your use-case, you might want to use the node distribution of maplibre-gl-native instead https://www.npmjs.com/package/@maplibre/maplibre-gl-native

@geraldo
Copy link
Contributor

geraldo commented Sep 22, 2022

The use case would be including Maplibre as dependency, for example offering a maplibre renderer in Maputnik or other kind of web apps depending on Maplibre. So it still would be web and not Android nor iOS, that's why I think using maplibre-gl-native would not work right?

@birkskyum
Copy link
Member Author

birkskyum commented Sep 22, 2022

If it's rendering the vector map in the browser, gl-js is the place to go. If it's server-rendering, the gl-native can be used.

@HarelM HarelM added enhancement New feature or request PR is more than welcomed Extra attention is needed labels Oct 21, 2022
@birkskyum
Copy link
Member Author

Related to maplibre/maplibre#159 through tree-shaking

@HarelM
Copy link
Collaborator

HarelM commented Mar 6, 2024

I have made a lot of change in version 4 and basically removed the global object that existed before.
It might be interesting to try out again to export the library in both esm and common js.

@HarelM
Copy link
Collaborator

HarelM commented May 10, 2024

The article which I'm looking at right now is the following:
https://www.sensedeep.com/blog/posts/2021/how-to-create-single-source-npm-module.html

Since we add a package.json file to dist folder in order to force bundling tools to treat maplibre as common js I think we'll need to place the esm output in a folder called mjs or something to avoid it being treated as commonjs.

I'll do some experiments locally and see if I can get to a working version or not.
Since this is not a replacement and only an addition there's no breaking change (hopefully) and I'll be able to push it to a regular version and not a breaking change version. later on we can decide if and when to deprecated commonjs build output.

@geraldo
Copy link
Contributor

geraldo commented May 10, 2024

Seems to work now as recent Maputnik versions (v2.0) use MapLibre 4.1.2 as dependency.

@HarelM
Copy link
Collaborator

HarelM commented May 12, 2024

Below is a link to an article on esm with rollup and worker configuration:
https://justinribeiro.com/chronicle/2020/07/17/building-module-web-workers-for-cross-browser-compatibility-with-rollup/
This is to overcome the amd usage for the code to load it to the worker.
The above suggest to export the worker code as text - which means no reuse of shared code between the worker and the main thread.
If there's no code sharing then the csp approach is probably better.
Seems like esm and loading the worker code automatically are contradicting one another.
So the current build configuration is the best solution as far as I understand from bundle size perspective.
We might consider introducing esm to the csp build, but this will have very small impact as I believe most users are not using the csp build.

If anyone has more information on this or knows this topic more than I do please feel free to let me know.

@HarelM
Copy link
Collaborator

HarelM commented May 12, 2024

@timocov sorry to tag and feel free to unsubscribe, but it would be great if you can take a peak at this and let me know if I'm missing anything - I know you have a lot of knowledge/experience around rollup, build, esm, commonjs amd etc, and any input would be helpful here.

@wmertens
Copy link

wmertens commented Sep 11, 2024

Am I understanding correctly that most of the code size is inside the worker? And that all of the code is used?

In that case ESM builds wouldn't make much of a difference.

It would be different if lots of worker code is not used, in that case it would be better to use dynamic imports and a cjs build for it, but still not ESM.

If most of the code size is on the main thread, ESM would help.

@HarelM
Copy link
Collaborator

HarelM commented Sep 11, 2024

There's a lot (I think) of shared code between the workers and the main thread, which is what the current build try to improve in terms of package size - i.e. pack the shared code once to be used by both the worker and the main thread without increasing the package size.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request PR is more than welcomed Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants