-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Switch dev environment to native ESM to support Node 12+ #10367
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @mourner ! this is a big lift!
Just a few nits but looks good otherwise!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks good. Just a couple questions
I had to leave a type-less package.json ({}) to satisfy both Webpack and Node.
Is there any risk in keeping a file in the dist/
folder around now - previously it was easy to clean this folder before a build to guarantee a clean slate. Secondly - does this package.json make things harder if we decide to ship 2-3 different forms of the main bundle - es module, umd, and an es5-compliant variation?
cc @mapbox/studio and @mapbox/map-design-api for the change to style spec bundle.
Currently the
I'd worry about it when we do ship more bundles since we can change the setup at the time. The ES module bundle would either live in a different folder, or have an |
Closes #10289. This turned out to be quite a bit more involved than I anticipated, but I finally have a fully green build! Documenting the most notable gotchas that I encountered:
"type": "module"
field was added topackage.json
to make sure every.js
file in the project is treated as ESM. However, to support usingimport mapboxgl from 'mapbox-gl'
orrequire('mapboxgl')
from Node, the UMD bundles including the main entry point (dist/mapbox-gl.js
) need to be treated as CommonJS, so this required adding a{"type": "commonjs"}
package.json
in thedist
folder (Node resolution determines the type going through parent folders until it finds apackage.json
). This brokewebpack
bundling for unclear reasons, but Node docs state that it will treat files ascommonjs
iftype
isn't specified, so I had to leave a type-lesspackage.json
({}
) to satisfy both Webpack and Node.Some tools that accept
.js
as a config format don't yet support ESM — they internally userequire
, which only supports CommonJS modules. Forpostcss
, I fixed this by renaming the config file to.cjs
. This didn't work fortestem
, which hardcodes the extension, so I had to move it to its own folder and add a{"type": "commonjs"}
package.json
just for it. We can simplify this once my tiny testem PR lands. Accept .cjs files as configs testem/testem#1416testem
only supportscjs
configs, but we need to import ES modules inside of one — the only way to do that is by using dynamicimport
—await import('<module.js>')
. All such imports have to wrapped in an async function to be ableawait
on it before using those modules so that it works on Node versions without unflagged top-level await support.ES modules have a few differences with CommonJS in terms of what's available top-level. One such difference is no
__dirname
and__filename
. The idiomatic solution is to implement such variables throughimport.meta.url
:However, Flow doesn't support
import.meta
— facebook/flow#6913. So I had to use a super-ugly hack that silences it in such places:The other tool that had problems with
import.meta
is ourflow-remove-types
fork. Fortunately this was easy to fix by releasing a new version of the fork. We can't switch to the officialflow-remove-types
tool from Facebook because it's roughly 4–5 times slower (using a parser compiled to JS from OCaml instead of Babel which it used when we forked it).Some uses of
require.resolve
can't be directly substituted with a ESM equivalent (such as when they resolve modules by name rather than path), so we have to use this:To be able to natively load ES modules that contain Flow types and GLSL and JSON imports, I wrote a custom Node loader (passed as
node --experimental-node-loader ./build/node-loader.js
with corresponding hooks. Node has it's own JSON import implementation behind--experimental-json-modules
, but it doesn't support named imports, so if we used it, we wouldn't be able to tree-shake unused properties. So I had to add a custom loader for JSON as well, although thankfully all the work converting JSON to ES modules is done bydataToEsm
function of@rollup/pluginutils
, same one that powersrollup-plugin-json
used for our bundle.When importing the bundle through
await import('mapboxgl')
, initially I got errors aboutself
being undefined. This got fixed by upgrading to the latest version of Rollup, which updates the bundle prelude to address this. So I updated all the Rollup plugins to their latest versions as well.Since the collection of shaders is looked up dynamically in the code (
shaders[name]
), shaders can't be named exports — things like this need to be converted to exporting a corresponding default object with properties. The oldshaders/index.js
that was used in Node context before was removed since we can now import shaders directly with the custom loader.The Circle CI image was updated to one with a newer Node version — 14.15. Local scripts such as ones generating code were all converted to ESM too. Node v10 no longer works for local development. Node v12
theoretically should workis confirmed to work too.Launch Checklist
mapbox-gl-js
changelog:<changelog>Fix dev environment to native ES modules to support Node 14+</changelog>