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

Start moving third party libraries to use npm #9706

Merged
merged 41 commits into from
Aug 23, 2021
Merged

Conversation

ebogo1
Copy link
Contributor

@ebogo1 ebogo1 commented Aug 1, 2021

This is a chunk of progress towards #9473.

Comparing the generated .zip files with the one in main after #9705 gets merged:

  • 64.1 MB in this branch vs 63.6 MB in main
  • In Build/Cesium/:
    • Cesium.js is 3.9 MB in this branch vs 3.7 MB in main (uncompressed)
    • ThirdParty/Workers/ now only has basis and draco files (we removed deflate/inflate.js and ktx-parse-modern.js)
  • In Source/ThirdParty/:
    • This folder is 3.6 MB up from 3.1 MB in main (uncompressed)
    • The new files generated from npm modules seem correct; they share the same folder as the third party files currently in main

Comparing the .tgz with main:

  • 22.0 MB here vs 21.4 MB in main
  • decodeGoogleEarthEnterprisePacket is about 1.5x as big in this branch; probably because we're importing more than we need from pako..

ebogo1 and others added 30 commits April 8, 2021 16:41
Remove `sprintf` third party library
Rather than submit libraries to Source/ThirdParty, which always end up
being modified for our build system and quickly go out of date, this
change starts to use libraries via npm instead. Currently Autolinker,
earcut, when, tween.js, rbush, kdbush, quickselect, and topojson are
ported.

The main hurdle that prevented us from doing this sooner was the fact that
Cesium has a long history of not requiring a build step after every code
change and has a goal of writing valid JS code, unlike many libraries today
 that mandate a bundler to turn invalid JS code into valid code.

Rather than mandate a bundler during development, this initial step adds a
"buildThirdyParty" function to the current "build" step. This function runs
third party libraries (defined in the ThirdParty/npm/ folder) through
RollUp and creates an equivalent file in `Source/ThirdPartyNpm`.

The change to end users will be non-existent, especially since the combined
Cesium.js will still re-export any third party modules as part of the
private API just like it used to.

This doesn't prevent code duplication for users using some of the same
third party libraries as Cesium. This is just an improvement as to how
depend on third party libraries internally.

I think Cesium's days of being "bundler free" are probably limited long
term, performance is the main hurdle and newer tools like esbuild may make
that no longer a problem. But that's outside the scope of this initial
goal.
Start of replacing submitted third party libraries with npm modules
Use npm for third party libraries
Move Gltf-Pipeline to Source/Scene
Move FXAA3_11 shader into Source/Shaders
@ebogo1 ebogo1 requested a review from mramato August 1, 2021 17:09
@cesium-concierge
Copy link

Thanks for the pull request @ebogo1!

  • ✔️ Signed CLA found.
  • ❔ 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.

CHANGES.md Outdated
@@ -6,6 +6,7 @@

- Added `ImageryLayerCollection.pickImageryLayers` which determines the imagery layers that are intersected by a pick ray. [#9651](https://github.com/CesiumGS/cesium/pull/9651)
- Added a `polylinePositions` getter to `Cesium3DTileFeature` that gets the decoded positions of a polyline vector feature. [#9684](https://github.com/CesiumGS/cesium/pull/9684)
- Started moving over third party libraries to use npm modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this the right place to mention the change? I think there should be a note of this somewhere in CHANGES.md.

Copy link
Contributor

Choose a reason for hiding this comment

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

This has zero user facing implications at this time, while we are moving to npm, there are still build time dependencies, not runtime dependencies (i.e. users who use Cesium don't actually have a direct dependencies on these libraries via npm)

Move ThirdPartyNpm files into ThirdParty
@@ -0,0 +1,2 @@
import pako from 'pako';
export { pako as default };
Copy link
Contributor

Choose a reason for hiding this comment

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

If all we use is pako-inflate, would it be better to only include inflate here? Then make sure roll-up treeshakes and it should create a smaller file.

Copy link
Contributor

Choose a reason for hiding this comment

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

treeshaking may not work, but pako also includes an inflate only dist.. looks like we can save over a hundred KB by using that instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be as simple as going back to inflate only like:

import pako_inflate from 'pako/lib/inflate.js';
export { pako_inflate as default };

and then update the calling code to just call pako_inflate instead of pako.inflate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I'm wrong.. you literally don't need to change the calling code.. just do

import pako from 'pako/lib/inflate.js';
export { pako as default };

and you end up with a pako object that only has inflate related stuff.

@mramato
Copy link
Contributor

mramato commented Aug 2, 2021

@ebogo1 it would be better to look at the size of the actual combined Cesium.js files and the files in workers.. the size of the zips, while indicative of size differences, don't really tell us what users are pulling down to use Cesium over the network.

@mramato
Copy link
Contributor

mramato commented Aug 2, 2021

Looks like we added ~512KB to the size to the combined/unminified Cesium.js and then a similar amount to the Workers, this is most likely caused by including way more of some third party libraries than we used to (like pako). It's probably easy to fix,but do we have any idea as to which libraries are causing the issue?

@lilleyse
Copy link
Contributor

lilleyse commented Aug 3, 2021

I updated gltf-pipeline in dc2e925. gltf-pipeline now uses prettier, though it uses a different version of prettier than CesiumJS so I couldn't remove it from .prettierignore.

@ebogo1 I tagged you to review CesiumGS/gltf-pipeline#600 which has some fixes for generating these files for CesiumJS.

@ebogo1
Copy link
Contributor Author

ebogo1 commented Aug 4, 2021

After importing less from pako and zip the unminified Cesium.js size went down to by about 300 kB to 11.1 MB and the Workers folder is now the same size as the one in the 1.84 release. Cesium.js.map is still about 500 kB bigger than in the release but is down 600 kB after b962003.

@ebogo1
Copy link
Contributor Author

ebogo1 commented Aug 16, 2021

I looked into the file size discrepancy more and don't think there's an easy solution:

  • Almost all of the bloat is coming from @zip.js/zip.js. Even though version 2.0.0 on npm is significantly smaller (unpacked size 744 kB vs 2.4 MB in the version here), downgrading the version had no effect on the unminified Cesium.js or Cesium.js.map sizes..
  • Per the README in the package's dist/ folder, I don't think there's a smaller import we can use. The npm package's lib/core/ folder is ~300 kB, so I'd guess it's responsible for most of the file size.

@mramato I could be wrong but is the only alternative here to try a different zip npm library?

As of the last commit:

  • Unminified Cesium.js is up to 11.1 MB from 10.9 MB in the 1.84 release (or 11.0 MB without npm zip.js)
  • Unminified Cesium.js.map is up to 19.8 MB from 19.3 MB in the 1.84 release (or 19.4 MB without npm zip.js)

@mramato
Copy link
Contributor

mramato commented Aug 22, 2021

All in all, this is okay with me as an incremental step. But it's made it more clear than ever that we need to bite the bullet and migrate to "live" compilation where Cesium dev doesn't function without tooling building valid JS in the background. The industry and moved on and we aren't doing our service by trying to hold on to the old ways. In particular esbuild would probably be a great fit for our needs and simplify this entire process.

I diffed main and this branch zip output and went through the combined Cesium.js diff by diff and nothing major jumped out. Everything appears to work locally as well.

I do have two questions:

  1. Did we revisit LICENSE.md to make sure it is up to date with any changed/updated libraries?
  2. Is there a plan to update the dependencies? A few of them are quite out of date.

Both can be handled in follow-up PRs. @lilleyse if you are okay with this feel free to merge.

@lilleyse
Copy link
Contributor

Thanks @ebogo1 and @mramato. The changes look good to me as well. I ran unit tests locally and spot checked some sandcastle examples.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants