Use minified Cesium in Node.js unless NODE_ENV=development #7514
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We know that removing debug code in Cesium can provide a substantial speed improvement, but previously the Node.js version would always use separate modules. This change allows Node.js to load the combined/minified version by default, for improved performance, and only loads the old separate modules if NODE_ENV=development (which is the standard mechanism for enabling extra debugging checks in most node packages.
|
Thanks for the pull request @mramato!
Reviewers, don't forget to make sure that:
|
|
@shunter any comments here? If now, should be ready to merge. |
|
|
|
Looks fine to me. Thanks @mramato |
Merged
mramato
added a commit
that referenced
this pull request
Feb 12, 2019
While the improvements in #7514 worked, they were a little more convoluted than they had to be and also caused some weird edge cases where loading Cesium in eval'd Node.js code didn't work. This change avoids using requirejs entirely when loading minified Cesium in Node and adds a smokescreen test to travis that fails in master. This also avoids the odd global workaround I had to put in place previously. I did have to modify the UMD headers for NoSleep.js and purify.js because their preference is to look for Node.js first and not use AMD if it's found, this was the root cause of the failures we were seeing in eval'd code. The smokescreen test I added will fail again if the problem were reintroduced. So overall, this make Cesium a little more node-friendly and just makes things cleaner from our end.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We know that removing debug code in Cesium can provide a substantial speed improvement, but previously the Node.js version would always use separate modules. This change allows Node.js to load the combined/minified version by default, for improved performance, and only loads the old
separate modules if NODE_ENV=development (which is the standard mechanism for enabling extra debugging checks in most node packages.
@shunter I was fumbling around with getting the minified version to load at all in node, my changes in
Source/main.jswere needed to get it to happen. As far as I can tell, this has no impact on browser-based usage; but can you take a look and let me know if there's a better approach here.Partially stems from discussion in #7508, but this is something we've tried in the past and I finally bit the bullet to figure it out.
CC @shehzan10