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

Switch dev environment to native ESM to support Node 12+ #10367

Merged
merged 15 commits into from
Feb 11, 2021
Merged

Conversation

mourner
Copy link
Member

@mourner mourner commented Feb 8, 2021

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:

  1. "type": "module" field was added to package.json to make sure every .js file in the project is treated as ESM. However, to support using import mapboxgl from 'mapbox-gl' or require('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 the dist folder (Node resolution determines the type going through parent folders until it finds a package.json). This broke webpack bundling for unclear reasons, but Node docs state that it will treat files as commonjs if type isn't specified, so I had to leave a type-less package.json ({}) to satisfy both Webpack and Node.

  2. Some tools that accept .js as a config format don't yet support ESM — they internally use require, which only supports CommonJS modules. For postcss, I fixed this by renaming the config file to .cjs. This didn't work for testem, 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#1416

  3. testem only supports cjs configs, but we need to import ES modules inside of one — the only way to do that is by using dynamic importawait import('<module.js>'). All such imports have to wrapped in an async function to be able await on it before using those modules so that it works on Node versions without unflagged top-level await support.

  4. 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 through import.meta.url:

import {fileURLToPath} from 'url';
const __dirname = fileURLToPath(new URL('.', import.meta.url));

However, Flow doesn't support import.metafacebook/flow#6913. So I had to use a super-ugly hack that silences it in such places:

// $FlowFixMe https://github.com/facebook/flow/issues/6913
const __dirname = fileURLToPath(new URL('.', import/*:: ("")*/.meta.url));
  1. The other tool that had problems with import.meta is our flow-remove-types fork. Fortunately this was easy to fix by releasing a new version of the fork. We can't switch to the official flow-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).

  2. 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:

import {createRequire} from 'module';
const require = createRequire(import.meta.url);
  1. 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 by dataToEsm function of @rollup/pluginutils, same one that powers rollup-plugin-json used for our bundle.

  2. When importing the bundle through await import('mapboxgl'), initially I got errors about self 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.

  3. 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 old shaders/index.js that was used in Node context before was removed since we can now import shaders directly with the custom loader.

  4. 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 work is confirmed to work too.

Launch Checklist

  • briefly describe the changes in this PR
  • update tests
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Fix dev environment to native ES modules to support Node 14+</changelog>

@mourner mourner self-assigned this Feb 8, 2021
@mourner mourner changed the title Switch dev environment to native ESM to support Node 14+ Switch dev environment to native ESM to support Node 12+ Feb 8, 2021
Copy link
Contributor

@arindam1993 arindam1993 left a 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!

dist/package.json Show resolved Hide resolved
test/integration/testem/testem.js Show resolved Hide resolved
Copy link
Contributor

@asheemmamoowala asheemmamoowala left a 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.

@mourner
Copy link
Member Author

mourner commented Feb 11, 2021

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.

Currently the prepare-publish script runs git clean -fdx for cleaning, which removes all files that are not tracked by git. Since this package.json is tracked, it won't be affected.

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?

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 .mjs extension to work with Node.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix Node 14 for development
3 participants