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

Workers as ESM, remove RequireJS #11400

Merged
merged 14 commits into from
Sep 1, 2023
Merged

Workers as ESM, remove RequireJS #11400

merged 14 commits into from
Sep 1, 2023

Conversation

ggetz
Copy link
Contributor

@ggetz ggetz commented Jul 6, 2023

Fixes #9024

Firefox added support for ESM workers as of 114 released in June. This was the last holdout among browsers for support of this feature.

When updating our build process, this prevented us from using ebuild to bundle workers, needing to fallback to rollup to bundle as AMD, and to include a copy of RequireJS which has security implications and does not play nicely with all bundlers.

Given the support in Firefox, we can now revisit how we handle workers and simplify things a bit.

  • Remove cesiumWorkerBootstrapper as it should no longer be needed given how import works in ESM modules
  • Finally removes RequireJS. The last place RequireJS was used in the codebase was hard-copied into web workers via cesiumWorkerBootstrapper in order to facilitate dynamic module loading.
  • Build the workers as ESM rather than AMD. This allows us to treat all workers similarly, therefore removing WorkersES6 directory. This also lets us bundle node_modules js code, like draco, directly rather than importing it at runtime.
  • Build (and rebuild) the ESM workers with esbuild rather than rollup to be consistant with the rest of the build process
    - We still bundle each worker individually with code splitting, and load the workers dynamically at runtime. I'd like to leave changing the distribution method to work for Inline Static Assets in Builds #10619 to keep the scope of this contained.
    • Include copyright banner, which wasn't previously implemented. This does bloat the size of the workers slightly.
  • Move all source code workers to Workers
  • There was also a weird typo in ThirdParty/Workers/basis_transcoder.js with an extra za appended which I can only assume was added accidentally that has been removed.
  • Refactor I3S* classes to better fit the pattern established by Draco and KTX2 decoding for using web workers with Web Assembly.

There is still a significant number of users that are not up-to-date on the compatible Firefox version. When using incompatible versions of FF, there following error is shown:
image

Stats

Worker build times

Measured by adding timing output around bundleWorkers.

Branch Build Time Rebuild Time
main 4.2s 4.2s
workers-as-esm 0.3s 0.1s

Worker built size

To measure this, I started built from a clean repository:

git clean -dxf
npm install
npm run make-zip

Sizes are the sum of the Build/X/Workers and Build/X/ThirdParty/Workers directories, where X is "CesiumUnminified" or "Cesium".

Branch Unminified Build Size Minified Build Size
main 3.3MB 0.92KB
workers-as-esm 2.3MB 0.89KB --> 0.98MB with copyright banner included

Minification here is at least constant with previous sizes, but it should be possible to get sizes of bundles down even further using terser directly.

While esbuild is not the optimal JavaScript minifier in all cases (and doesn't try to be), it strives to generate minified output within a few percent of the size of dedicated minification tools for most code, and of course to do so much faster than other tools.

ESBuild docs

TODO

  • Update CHANGES.md
  • Fix unit tests
  • Add unit tests for feature checking

@cesium-concierge
Copy link

Thanks for the pull request @ggetz!

  • ✔️ 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.

@ggetz ggetz marked this pull request as ready for review July 10, 2023 14:36
@ggetz ggetz requested a review from jjhembd July 10, 2023 14:36
Copy link
Contributor

@jjhembd jjhembd left a comment

Choose a reason for hiding this comment

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

Thanks @ggetz, it's nice to get rid of an old dependency!

How do I measure the worker built size? If I just run du -h on the Build/CesiumUnminified/Workers and /Build/Cesium/Workers paths, I get 2.3M for unminified, 1.1M for minified.

Other than that, I just have one question about the TestWorkers path.

@@ -1372,7 +1357,7 @@ export async function runCoverage(options) {
// Static assets are always served from the shared/combined folders.
{ pattern: "Build/CesiumUnminified/**", included: false },
{ pattern: "Specs/Data/**", included: false },
{ pattern: "Specs/TestWorkers/**", included: false },
{ pattern: "Build/Specs/TestWorkers/**", included: false },
{ pattern: "Specs/TestWorkers/**/*.wasm", included: false },
Copy link
Contributor

Choose a reason for hiding this comment

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

Most of these paths are changed from "Specs/TestWorkers" to "Build/Specs/TestWorkers". The remaining "Specs/Testworkers/**/*.wasm" pattern is only matching "Specs/Testworkers/TestWasm/testWasm.wasm", is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The .js files require a build, but the .wasm files are already built so can be included directly from their original directory. I'll add the .js extension to the blobs to be clear.

@@ -5,6 +5,7 @@ if [ $TRAVIS_BRANCH == "cesium.com" ]; then
else
npm --silent run make-zip
npm pack &> /dev/null
npm pack --workspaces &> /dev/null
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was added to ensure the parent cesium package installs the working version of each workspace package, not the official published version.

@ggetz
Copy link
Contributor Author

ggetz commented Aug 30, 2023

Thanks @jjhembd! This should be ready to go.

Copy link
Contributor

@jjhembd jjhembd left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @ggetz!

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.

Replace use of eval()
3 participants