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
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
"overrides": [
{
"files": [
"index.js",
"index.cjs",
"server.cjs",
"gulpfile.js",
"Source/Workers/transferTypedArrayTest.js"
Expand Down
4 changes: 2 additions & 2 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ script:

# Various Node.js smoke-screen tests
- node -e "const Cesium = require('./');"
- NODE_ENV=development node index.js
- NODE_ENV=production node index.js
- NODE_ENV=development node index.cjs
- NODE_ENV=production node index.cjs

- npm --silent run cloc
3 changes: 3 additions & 0 deletions Tools/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"type": "commonjs"
}
4 changes: 2 additions & 2 deletions gulpfile.js → gulpfile.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,7 @@ gulp.task('makeZipFile', gulp.series('release', function() {
'Specs/**',
'ThirdParty/**',
'favicon.ico',
'gulpfile.js',
'gulpfile.cjs',
'server.cjs',
'package.json',
'LICENSE.md',
Expand Down Expand Up @@ -457,7 +457,7 @@ function deployCesium(bucketName, uploadDirectory, cacheControl, done) {
'ThirdParty/**',
'*.md',
'favicon.ico',
'gulpfile.js',
'gulpfile.cjs',
'index.html',
'package.json',
'server.cjs',
Expand Down
41 changes: 41 additions & 0 deletions index.cjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/*eslint-env node*/
'use strict';

var path = require('path');

// If in 'production' mode, use the combined/minified/optimized version of Cesium
if (process.env.NODE_ENV === 'production') {
module.exports = require(path.join(__dirname, 'Build/Cesium/Cesium'));
return;
}

try {
module.exports = require('esm')(module)('./Source/Cesium.js');
} catch (e) {
if (e.code !== 'ERR_REQUIRE_ESM') {
throw e;
}

// Node 12+ throws an exception when trying to load an ES module via `esm`,
// because `esm` loads Cesium ES modules via require instead of import, and
// that is an illegal thing to do because Cesium's package.json declares
// `"type": "module"`. Node doesn't know that `esm` is magically
// transforming the ES modules to CommonJS.
//
// So, we override the loader for .js files to skip the exception.
// Idea from here:
// https://github.com/standard-things/esm/issues/855#issuecomment-566825957
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.

if (filename.startsWith(__dirname)) {
var content = fs.readFileSync(filename, 'utf8');
module._compile(content, filename);
return;
}
originalFunc(module, filename);
};

module.exports = require('esm')(module)('./Source/Cesium.js');
}
11 changes: 0 additions & 11 deletions index.js

This file was deleted.

62 changes: 33 additions & 29 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,12 @@
"url": "https://github.com/CesiumGS/cesium/issues",
"email": "cesium-dev@googlegroups.com"
},
"main": "index.js",
"module": "Source/Cesium.js",
"main": "index.cjs",
"module": "./Source/Cesium.js",
"exports": {
"require": "./index.cjs",
"import": "./Source/Cesium.js"
},
"type": "module",
"dependencies": {
"esm": "^3.2.25"
Expand Down Expand Up @@ -81,35 +85,35 @@
"yargs": "^15.0.1"
},
"scripts": {
"convertToModules": "gulp convertToModules",
"convertToModules": "gulp -f gulpfile.cjs convertToModules",
"start": "node server.cjs",
"startPublic": "node server.cjs --public",
"build": "gulp build",
"build-watch": "gulp build-watch",
"buildApps": "gulp buildApps",
"clean": "gulp clean",
"cloc": "gulp cloc",
"combine": "gulp combine",
"combineRelease": "gulp combineRelease",
"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.

"build-watch": "gulp -f gulpfile.cjs build-watch",
"buildApps": "gulp -f gulpfile.cjs buildApps",
"clean": "gulp -f gulpfile.cjs clean",
"cloc": "gulp -f gulpfile.cjs cloc",
"combine": "gulp -f gulpfile.cjs combine",
"combineRelease": "gulp -f gulpfile.cjs combineRelease",
"coverage": "gulp -f gulpfile.cjs coverage",
"generateDocumentation": "gulp -f gulpfile.cjs generateDocumentation",
"generateDocumentation-watch": "gulp -f gulpfile.cjs generateDocumentation-watch",
"eslint": "eslint \"./**/*.js\" \"./**/*.html\" --cache --quiet",
"makeZipFile": "gulp makeZipFile",
"minify": "gulp minify",
"minifyRelease": "gulp minifyRelease",
"release": "gulp release",
"build-specs": "gulp build-specs",
"test": "gulp test",
"test-all": "gulp test --all",
"test-webgl": "gulp test --include WebGL",
"test-non-webgl": "gulp test --exclude WebGL",
"test-webgl-validation": "gulp test --webglValidation",
"test-webgl-stub": "gulp test --webglStub",
"test-release": "gulp test --release",
"sortRequires": "gulp sortRequires",
"deploy-s3": "gulp deploy-s3",
"deploy-status": "gulp deploy-status",
"deploy-set-version": "gulp deploy-set-version"
"makeZipFile": "gulp -f gulpfile.cjs makeZipFile",
"minify": "gulp -f gulpfile.cjs minify",
"minifyRelease": "gulp -f gulpfile.cjs minifyRelease",
"release": "gulp -f gulpfile.cjs release",
"build-specs": "gulp -f gulpfile.cjs build-specs",
"test": "gulp -f gulpfile.cjs test",
"test-all": "gulp -f gulpfile.cjs test --all",
"test-webgl": "gulp -f gulpfile.cjs test --include WebGL",
"test-non-webgl": "gulp -f gulpfile.cjs test --exclude WebGL",
"test-webgl-validation": "gulp -f gulpfile.cjs test --webglValidation",
"test-webgl-stub": "gulp -f gulpfile.cjs test --webglStub",
"test-release": "gulp -f gulpfile.cjs test --release",
"sortRequires": "gulp -f gulpfile.cjs sortRequires",
"deploy-s3": "gulp -f gulpfile.cjs deploy-s3",
"deploy-status": "gulp -f gulpfile.cjs deploy-status",
"deploy-set-version": "gulp -f gulpfile.cjs deploy-set-version"
}
}