-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Streamline build processes with esbuild #10399
Conversation
Thanks for the pull request @ggetz!
Reviewers, don't forget to make sure that:
|
@sanjeetsuhag I'm still making a few tweaks based on the final TODO's, but most of this PR should be static. Could you give this a try and let me know if you have any general feedback? |
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.
@ggetz It's amazing how much faster all the scripts are working. Can't wait to see how much of an impact this will have on CI. The debugging workflow for workers is much easier now too! Otherwise, I just had a few comments.
Thanks @sanjeetsuhag! Your feedback should all be addressed. |
@sanjeetsuhag Could you please give the latest updates a round of review? Assuming this is in a good place, it would help to get this merged and move #10538 along as well. |
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.
@ggetz This is very close. I just had some minor comments. One big thing in running the code - it seems like the build
step is slow.
On this branch, I ran git clean -xdf; npm install; npm run build
and the build
step took 5.84s. I switched to main
and ran the same command and the build
step took 5.04s.
This is mainly due to reverting back to rollup to build the workers for the reasons detailed in the PR description. The up-front |
Thanks @sanjeetsuhag! This is updated. |
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.
Everything seems to be in good shape. This is an awesome improvement! Thanks @ggetz! We should also update the Release Guide since it references outdated steps like build-specs
.
- Built `Cesium.js` is no longer AMD format. This may or may not be a breaking change depending on how you use Cesium in your app. See our [blog post](TODO) for the full details. [#10399](https://github.com/CesiumGS/cesium/pull/10399) | ||
- If you were ingesting individual ESM-style modules from the combined file `Build/Cesium/Cesium.js` or `Build/CesiumUnminified/Cesium.js`, instead use `Build/Cesium/index.js` or `Build/CesiumUnminified/index.js` respectively. | ||
- `CESIUM_BASE_URL` should be set to either `Build/Cesium` or `Build/CesiumUnminfied`. | ||
- Built `Cesium.js` has gone from `12.5MB` to `8.4MB` unminified and from `4.3MB` to `3.6MB` minified. `Cesium.js.map` has gone from `22MB` to `17.2MB`. |
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 should probably be absolutely top of the list with 📣, it's not a breaking change and it's a huge deal.
@@ -5,6 +5,10 @@ | |||
##### Breaking Changes :mega: | |||
|
|||
- Tilesets and entities now use `ModelExperimental` by default. This can still be changed by setting `ExperimentalFeatures.enableModelExperimental = false`. [#10530](https://github.com/CesiumGS/cesium/pull/10530) | |||
- Built `Cesium.js` is no longer AMD format. This may or may not be a breaking change depending on how you use Cesium in your app. See our [blog post](TODO) for the full details. [#10399](https://github.com/CesiumGS/cesium/pull/10399) | |||
- If you were ingesting individual ESM-style modules from the combined file `Build/Cesium/Cesium.js` or `Build/CesiumUnminified/Cesium.js`, instead use `Build/Cesium/index.js` or `Build/CesiumUnminified/index.js` respectively. |
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.
Doesn't Cesium now 100% require a bundler to use modules? (And a bundler that allows for node resolution rules at that). Or are we still embedding third party code?
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.
We're embedding the third-party code in these builds. In the blog post I mention that production apps that use their own bundler should be using modules directly Source
, which does not have third party code bundled. Which is what we've previously recommended as well if I understand correctly.
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.
We have previously recommended it, but since we embedded ThirdParty code in source, you didn't need a bundler to use them. Now you do.
Another problem I just noticed is that package.json
lists everything as a dev dependency, which may not be correct because that means if I npm install Cesium
and then try to use Source
it's going to pull in modules that don't exist because they are not a dependency
or peerDependency
.
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.
Gotcha, I see the distinction. For the point about the bundler, I will update both CHANGES.md
and the blog post with the requirement.
For the latter, I will package.json
such that modules in Source/ThirdParty
are now marked as direct dependencies. Sound correct?
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.
Yes. It's unfortunate because people who want to use the bundled Cesium.js still need to sync those dependencies but there's no other way that I know of to do it and have all usage-types work.
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.
Hey all, speaking of package.json
, would this be a good time to address #9212 as well? It's really important to get the exports right, I had a workaround but had to get rid of it to be able to use another important dependency at all...
ETA: to clarify, any resource that you intend for people to be able to bundle or extract separately (CSS, images, maybe data files like terrain height / IAU JSON) should have its own exports
entry, and excluding those entries makes consumption of the resources effectively impossible.
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.
Does this or will this provide any mechanism to preload the workers up front? Loading ~50 web worker files on our network kills us by delaying user polygon drawings. |
I was going to say that your application might be able to improve the situation using |
@ashley-mort If you are using the Primitive API, there's a We're also exploring optionally inlining static assets in a single distributable, which would include the web workers with the initial CesiumJS download. |
The inlining static assets in a single distributable seems like it would be great! I have tried out the c137.js stuff and it was a huge improvement regarding the time it takes to display user created polygons on the network we have. |
Fixes #10373
Fixes #10400
Fixes #6464
Fixes #9906
Fixes #8245
Fixes #6993
Fixes #9388
Should fix #7669, verification needed
Workaround for #9416
How to build
For a single build, the workflow is the same as before:
For an ongoing build task which incrementally rebuilds based on file changes, run:
and as a sperate process run:
For this to be truly a "just in time" build such that the build is run on
npm start
, we would want to run the equivalent ofbuild-watch
inserver.js
. In my mind at least, that would require a bit of a refactor ofgulpfile.cjs
so that the common build functions (a majority of the file) is either factor out into a separate file, or exported. Given we want to follow up this PR with revisions to the build scripts such as naming, I thinks it best to defer that change to a subsequent PR.build
now encompasses the functionality of bothcombine
andbuild-specs
, so they have been removed.Ideally, for both simplicity and performance, we we use esbuild for workers as well, but we see the end result is significantly increased in size (1.1MB -> 10.1MB minified). I believe this is because there's no code splitting going on. However, code splitting is not supported for IIFE with esbuild, and Firefox still does not support ESM modules in workers. Potentially, we could output ESM with code splitting, and then post-process with rollup. This PR leaves building workers as is. When the Firefox support is there, it may make sense to holistically address our worker strategy.
Performance Stats
main
build
(previouslycombine
+build-specs
)build-watch
rebuildbuild --minify --pragmas
(minifyRelease
)buildApps
Size Stats
main
Highlights
node_modules
directly@legal
comments are not removed during bundling or minification.build-watch
task, only rebuilding certain aspects depending on the file changedconvertToModules
. This task was put in place for the ES6 module transition a few years ago, and we don't have a need to maintain it going forward.Source
code directly. Instead, they use the unminified IIFE build with source maps by default. Despite this, debugging still works as before since the debugger will handle the mapping back to the original source code. Therefor, there's no longer any need to have abuild
option or query anymore. Therelease
option instead loads the minified IIFE build with pragmas removed instead, pretty much the same as before.engine
field topackage.json
to warn about this.Source
modules, we no longer build the workers to theSource/Workers
directory. If a user want to do this, they should see theCESIUM_BASE_URL
to one of the built directories. I'm not 100% sure this is the right decision.Known TODOS:
makeZipFile
task to reflect new filesbuild-watch
taskCHANGES.md