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

Configure zip.js with pako workers #9861

Merged
merged 14 commits into from
Oct 11, 2021
Merged

Configure zip.js with pako workers #9861

merged 14 commits into from
Oct 11, 2021

Conversation

ebogo1
Copy link
Contributor

@ebogo1 ebogo1 commented Oct 8, 2021

Fixes CesiumGS/3d-tiles-validator#215.
Fixes CesiumGS/gltf-pipeline#603.
Fixes CesiumGS/obj2gltf#266.

This should also make CesiumJS work with Node 16.

All the above issues came from the configureWebWorker function, which is included in most versions of zip.js except for zip-no-worker.js. This version of zip.js depends on external inflate and deflate workers, which luckily we already have from pako. To put them in a location that's accessible in built/packaged CesiumJS, I added some steps to the prepare task to copy the required files from node_modules.

I think this is merge-able as is, but it'd be nice to get a single place to run zip.configure({ ... }). In 1ff47b6 I tried doing this with Source/Core/zipPakoConfig.js, but this did not work with built versions of Cesium and I haven't figured out why yet.

This does not fix the issues opened in CesiumJS about zip.js regressions in 1.85, but for now I believe all of those are related to webpack 4 not working with worker imports out of the box (webpack 5 does).

@cesium-concierge
Copy link

Thanks for the pull request @ebogo1!

  • ✔️ 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.
  • ❔ Changes to third party files were made.
    • Looks like a file in one of our ThirdParty folders (ThirdParty/, Source/ThirdParty/) has been added or modified. Please verify that it has a section in LICENSE.md and that its license information is up to date with this new version.
  • ❔ 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.

@ebogo1 ebogo1 requested a review from lilleyse October 8, 2021 23:16
@lilleyse
Copy link
Contributor

I confirmed that the KMZ related sandcastles work and unit tests pass locally, both in local development and with the built version. I also confirmed that obj2gltf and other node libraries that use the cesium package work in Node 16 again. No notable change in the built Cesium.js size.

I think this is merge-able as is, but it'd be nice to get a single place to run zip.configure({ ... }). In 1ff47b6 I tried doing this with Source/Core/zipPakoConfig.js, but this did not work with built versions of Cesium and I haven't figured out why yet.

I'm ok with the current approach, at this point it's better to merge and move on to other things. @ebogo1 could you start a 1.86.1 section in CHANGES.md and add a note that this fixes a regression in the cesium npm package when using Node 16.

@mramato
Copy link
Contributor

mramato commented Oct 11, 2021

I already spoke to @ebogo1 about this offline but having a single configuration point should be trivial by just creating a single Core/zip that does a one time configuration. Then clients import that instead of the third party directly. That's exactly how knockout works as well and other libraries throughout our code bases that have configuration that needs to be done before use.

@lilleyse
Copy link
Contributor

Merging, @ebogo1 opened #9863 for future cleanup here

@lilleyse lilleyse merged commit 7067c04 into main Oct 11, 2021
@lilleyse lilleyse deleted the zip-pako branch October 11, 2021 21:09
@ebogo1 ebogo1 mentioned this pull request Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants