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

Fix Cesium in Node.js 12+. #8659

Merged
merged 11 commits into from
Mar 19, 2020
Merged

Fix Cesium in Node.js 12+. #8659

merged 11 commits into from
Mar 19, 2020

Conversation

kring
Copy link
Member

@kring kring commented Mar 4, 2020

Node.js 12+ requires that ES modules are loaded with import rather than require, and it throws an exception if you get this wrong. An ES module is any module that is named .mjs or whose closest package.json specifies "type": "module" (as Cesium's does).

The Node.js module docs explain a couple of approaches for making a single package that works when imported as either an ES module or as a CommonJS module. But the first requires that the library be natively CommonJS (which Cesium is not), and the second requires two copies of the library (one CommonJS modules, one ES modules) and a dodgy approach to allow the two to share state 🙄. There's no scenario where the library is natively ES modules and can be loaded in CommonJS apps using esm, and short of terrible hackery it appears that this is no longer possible to achieve in Node 12+.

So, this PR does some terrible hackery, as suggested here:
standard-things/esm#855 (comment)
(By the way, the commentor above implemented the check and exception that is causing us problems, as well as large parts of Node's module system in general I believe, so presumably knows what he's talking about! nodejs/node#29492)

This PR also adds a package.json with "type": "commonjs" to the Tools directory so that the .js files there are treated as CommonJS. This is needed for several of our build tools to work in Node 12+. It also renames gulpfile.js to gulpfile.cjs so that file isn't interpreted as an ES module, and it adds an explicit parameter to all the npm run scripts telling gulp to use it. gulp isn't (yet?) able to look for a gulpfile.cjs itself.

Other approaches we could take, not implemented in this PR:

  • Remove "type": "module". If we do that, we don't need any of the other changes in this PR and everything will work great in Node 12+. BUT users won't be able to import Cesium as "real" ES modules using --experimental-modules anymore. They'll always have to use esm.
  • Ship separate CommonJS and ES builds of Cesium in the npm package. Name the files .mjs or .cjs as appropriate (or put a separate package.json in each version's subdirectory with a different "type"). Ideally break all of Cesium's shared state into separate CommonJS modules as described in Approach 2 in the node.js docs above. That way if one thing imports the CommonJS version and another thing imports the ES version, will "only" have two versions of all the code rather than everything breaking cause there's two versions of the global data.

Fixes #8452

@cesium-concierge
Copy link

Thanks for the pull request @kring!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@kring
Copy link
Member Author

kring commented Mar 4, 2020

By the way, be very careful when testing anything with esm. Once it successfully loads a file (e.g. because you changed something), it will cache the version of the file converted to CommonJS. Then future loads of the same file will use the cached version, which Node.js considers to be the right module type. So if you:

  1. Make a change such that your test Cesium module loads successfully with esm (e.g. remove "type": "module" from package.json.
  2. Run the app once so it gets loaded.
  3. Make another change such that now the Cesium module should not load successfully because of the ERR_REQUIRE_ESM error (e.g. re-add "type": "module" to package.json).
  4. Run the app again.

The app will run successfully until you rm -rf node_modules/.cache/esm! Ask me how much time that cost me. Actually, please don't. I don't want to talk about it.

@kring
Copy link
Member Author

kring commented Mar 4, 2020

I made a project to test some different scenarios for importing Cesium in node.js:
https://github.com/kring/cesium-in-node-scenarios

@emackey
Copy link
Contributor

emackey commented Mar 4, 2020

This branch built for me under Node 12 LTS. The npm install phase was pretty noisy with warning messages, but the build seems OK.

I got about 37 warnings from npm install all related to the fsevents module, that all look like this but with assorted filenames on the end:

npm WARN enoent SKIPPING OPTIONAL DEPENDENCY: ENOENT: no such file or directory, rename '.\node_modules\chokidar\node_modules\fsevents\node_modules\yallist' -> '.\node_modules\chokidar\node_modules\fsevents\node_modules.yallist.DELETE'

@kring
Copy link
Member Author

kring commented Mar 4, 2020

Thanks for taking a look, Ed. I think those warnings happen even in Node 10, don't they?

@emackey
Copy link
Contributor

emackey commented Mar 4, 2020

I may have skipped 10 on my way here, sorry... 😄

@mramato
Copy link
Contributor

mramato commented Mar 5, 2020

Yeah, npm install is noisy and terrible. Those are in 10 as well. I will hopefully have some time to look over this soon.

index.cjs Outdated
var fs = require('fs');

var originalFunc = require.extensions['.js'];
require.extensions['.js'] = function(module, filename) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a global change to the node environment, right? Sounds like we don't have a lot of options, just wanted to confirm.

Just to throw out an alternative, we could always use the built version of Cesium in NodeJS and by requiring CesiumUnminified/Cesium.js we would still get extra debugging and such.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, if esm is going the way of the dodo, maybe it's better to just use combined in all cases now and we can revisit going full import later on (Node 14?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea! I got rid of the hack and made index.cjs always load the built version, either Build/Cesium/Cesium.js or Build/CesiumUnminified/Cesium.js depending on the NODE_ENV.

I also hooked up sourcemaps so it still looks like separate source file while debugging. I couldn't get it to work in the minified build for some reason, though. Uglify/Rollup are supposed to be able to do sourcemapping in this scenario, but the generated sourcemaps were rubbish. Step into a Cesium function in a release build and you'd end up somewhere totally random instead of in the function you're actually in.

If you use Node 12 and proper ES module support, you'll use the ES modules directly, with no transpilation by esm or other shenanigans. However, the debug pragmas won't be removed in this scenario. That's unfortunate, but a problem for another day, I think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks again for your work on this. We talked offline, but just to summarize for anyone else, rollup-plugin-strip-pragma can be used for pragma removal, debug sourcemaps are still enabled, release will have to wait for a different PR to figure out the problem.

package.json Outdated
"coverage": "gulp coverage",
"generateDocumentation": "gulp generateDocumentation",
"generateDocumentation-watch": "gulp generateDocumentation-watch",
"build": "gulp -f gulpfile.cjs build",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can revert all of the -f gulpfile.cjs and just tell gulp to use it by default, see https://github.com/CesiumGS/cesium/pull/8541/files#diff-a663dcab4db6e595455e0c837b86ee03

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, done.

@mramato
Copy link
Contributor

mramato commented Mar 11, 2020

Thanks for the exemplary PR description and details @kring.

Pretty sure I'm okay with all of this except for the those couple of questions/comments.

We could potentially do a node-only release of Cesium with this fix once it's in master since it's affecting a lot of people at this point

@tmcintire
Copy link

I'd love to see this merged in, we could really use this change! :)

@mramato
Copy link
Contributor

mramato commented Mar 18, 2020

@tmcintire this will definitely go out with the next release on April 1. @kring do you have time to help finish this up? If not I can take it over.

@kring
Copy link
Member Author

kring commented Mar 18, 2020

@kring do you have time to help finish this up? If not I can take it over.

Yep, thanks for the reminder! I'll do it today.

@kring kring mentioned this pull request Mar 19, 2020
@mramato
Copy link
Contributor

mramato commented Mar 19, 2020

Everything looks great, just want to do some final testing once CI passes and than I'll merge.

@mramato
Copy link
Contributor

mramato commented Mar 19, 2020

Did a quick test with node 12 and 13 and was able to import Cesium in both cases as both commonjs or ES modules. (Node 12 still required a flag, 13 did not).

Also tested a rollup based app using node-resolve with Cesium ES6 and that worked as well.

Going to merge so this has some time to stew in master.

Thanks again @kring!

@mramato mramato merged commit f36a25c into master Mar 19, 2020
@mramato mramato deleted the node12 branch March 19, 2020 13:24
@shunter
Copy link
Contributor

shunter commented Apr 1, 2020

@kring I noticed that after this PR the copyright banner is no longer included in the built minified files (for example, check today's release zip) despite using the banner option. Maybe that option doesn't work correctly with minified bundles?

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.

Error using cesium in Node 12 or 13
7 participants