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

Cesium ES6 Module Support #8122

Closed
17 tasks done
mramato opened this issue Sep 2, 2019 · 28 comments
Closed
17 tasks done

Cesium ES6 Module Support #8122

mramato opened this issue Sep 2, 2019 · 28 comments

Comments

@mramato
Copy link
Contributor

mramato commented Sep 2, 2019

While there was some discussion in #2524 related to ES6 module support, we never actually wrote up an issue to track it specifically. Below is my current best guess for a list of tasks we'll need to implement and questions to sort out. This is a living document, so I expect to update this description as we learn more.

Assumptions

  • The goal of this issue is to switch to ES6 modules, not allow for other ES6 features, such as async/await, that would require transpiling with Babel. We will tackle that as a separate enhancement once initial ES6 work is done.
  • We do not want to add any additional build processes for development, Chrome/Firefox/Edge/Safari should "just work" with the minimal build we have in place
  • We want to retain the ability to test against the combined build if possible.
  • IE11 and Node.js will require a build step to run, since IE 11 does not support ES6 modules and Node support is still experimental.
  • For users who currently use the combined Cesium.js file, nothing should change.
  • With ES6, there's no longer a reason to use Cesium modules directly. The official Cesium build will change to 4 options:
    • The traditional combined/minified Cesium.js
    • The traditional combined/debug Cesium.js
    • ES6 combined/minified Cesium.js
    • ES6 combined/debug Cesium.js

Prep work

Actual migration

  • Write script to convert individual modules to ES6. (Starting in es6-playground branch)
  • build task should output shaders and Cesium.js as ES6.
  • Refactor Workers to be based on ES6 instead of requirejs
  • Switch to roll-up based build system. (A lot of "sub-tasks" probably involved here as well)
    • Plugin to deal with pragma
  • Update CesiumViewer to a ES6/roll-up app.
  • Delete or update TimelineDemo
  • Update release zip generation to only include what's required.
  • Get unit tests running
  • Update Sandcastle (Stays ES5 but bucket uses ES6 Cesium?)
  • Update actual Sandcastle html demos and rename bucket
  • Figure out new zip packaging for ES6
  • Move Workers to WorkersES6 and Workers/Build to Workers
  • Test against the built version of Cesium

Open Questions

  1. Do we try and continue to provide AMD modules (at least temporarily) or do we make a clean break for ES6. This only affects people using our AMD modules without a system that supports ES6 (which is almost no one) so a clean break is my vote.
  2. Since Specs will be ES6 too, it may be impossible to unit test in IE11 without a completely different approach. This is probably okay since IE11 is not longer for this world.
  3. What are we forgetting?
@mramato
Copy link
Contributor Author

mramato commented Sep 2, 2019

While I hacked together a ~80% complete script for the actual module conversion in the es6-playground branch, it might be better to use an existing tool like https://www.npmjs.com/package/@buxlabs/amd-to-es6 for the task.

At first, I thought these tools were too limited, since we have a bunch of edge cases in the Cesium code but I wonder if a better solution could be to use amd-to-es6 first and then a second script to clean things up. For example, we'll want to remove use strict from everywhere, since ES6 modules are strict by default and our Spec files probably want to import the single Cesium.js fie instead of individual tests so that we can run against both the module and combined versions.

UPDATE: After playing with a few options, I think rolling our own is probably the best approach to avoid uneeded code formatting churn. Ultimately these scripts are "one and done" anyway.

@mramato
Copy link
Contributor Author

mramato commented Sep 4, 2019

roll-up has automatic shared code bundle generation when building multiple files at once (i.e. it would extract commonality between our workers and the primary Cesium.js file). This may greatly reduce overall file-size of loading Cesium apps in the browser if we use it properly.

@TJKoury
Copy link
Contributor

TJKoury commented Sep 4, 2019

The biggest issue with shared code bundle generation is that workers cannot use es6 syntax (yet). You are still stuck with using the 'import' statement.

@mramato
Copy link
Contributor Author

mramato commented Sep 4, 2019

Thanks, that does seem like a problem and a big limitation for Cesium's no-build goals during development. Some details here for anyone interested: https://developer.mozilla.org/en-US/docs/Web/API/Worker/Worker

A roll-up step for the workers that just produces non-ES6 module versions may be faster enough to get away with, but we'll have to try it out to see.

@markerikson
Copy link
Contributor

FWIW, I did a mass AMD->ESM codemod migration on a work app last fall. I documented my workflow here:

https://blog.isquaredsoftware.com/2018/11/git-js-history-rewriting/

The specific details on codemods are here:

https://blog.isquaredsoftware.com/2018/11/git-js-history-rewriting/#rewriting-js-source-with-codemods

And I put up a repo with the conversion scripts I wrote:

https://github.com/markerikson/git-js-repo-history-conversion-utils

In my case it was even more complicated because I was trying to rewrite the entire app history to look like the code had always been written in ES6 syntax.

@mramato
Copy link
Contributor Author

mramato commented Sep 10, 2019

Thanks @markerikson, will definitely check it out.

@mramato
Copy link
Contributor Author

mramato commented Sep 16, 2019

Update Sandcastle is going to be interesting. I think we can get away with keeping it a AMD app for now and it can continue to use it's ancient version of Dojo. But for sucking in CesiumJS itself, it can use ES6 in the bucket during development and then pull in the built version (similar to what it does now). May actually not be that bad or it can turn into a total nightmare. We probably won't know until we get in there.

@mramato
Copy link
Contributor Author

mramato commented Sep 16, 2019

I'm actually struggling a bit with how we may be able to run tests against the built version of Cesium the way we do now.

I think one option is to have all of the Specs point to Source/Cesium.js as an ES6 module and when running against individual modules, everything just works. Then if we want to run with the built version, you just build as normal and a command line flag would proxy the Karma server to return the combined/minified ES6 version of Cesium.js.

If that works, then to test against the non-ES6 combined/minified version, we could just have another auto-generated wrapper that exposes a ES6 module that just proxies to the built ES5 version.

I think that gets us to what we have with the current set up, but like everything else, we won't know until we try.

@TJKoury
Copy link
Contributor

TJKoury commented Sep 18, 2019

This is one of the big questions with our current es6 version. I’m just running a small subset of the combined tests against the built es6 file. It is hardly comprehensive.

@mramato
Copy link
Contributor Author

mramato commented Sep 24, 2019

For those that haven't been following along, we're in the home stretch for ES6 support!

One of the main open questions I'm trying to answer is how we ship ES6 (from a dist perspective). I think my preference is to not ship the Source directory in the zip at all and instead just ship a unified Cesium.js in ES6 format. Just like we have Cesium and CesiumUnminified we'll have CesiumES6 This should make it easy to use and also easy to deploy the correct pieces as part of a built/minified ES6 app.

Users would then write code like the below and tools can use tree-shaking to remove the code users don't need. This seems to work great via roll-up

import {
    ArcGISTiledElevationTerrainProvider,
    Math as CesiumMath,
    Viewer
} from './Cesium.js';

The alternative would be to ship the Source directory and have people require modules in directly. The only benefit here is that you are definitely only including what gets used, but it's way more verbose.

import ArcGISTiledElevationTerrainProvider from './Cesium/Source/Core/ArcGISTiledElevationTerrainProvider.js'
import CesiumMath from './Cesium/Source/Core/Math.js'
import Viewer from './Cesium/Source/Widgets/Viewer.js'

Does anyone have a strong preference or argument for one or the other? I'm definitely leaning towards the first one so far.

@mramato
Copy link
Contributor Author

mramato commented Sep 24, 2019

To clarify, the new packaging I'm suggesting something like

  • Build/Cesium - A UMD release build that can be included via a script tag, requirejs, or Node.js
  • Build/CesiumUnminified - A UMD debug build that can be included via a script tag, requirejs, or Node.js
  • Build/CesiumES6 - A ES6 roll-up build of Cesium ready for use in ES6 and minification by build tools like rollup and weback.

The Source directory goes away because it's no longer needed. Specs will go away too. I think the release zip size would shrink dramatically as well. This would be step one of streamlining the release zip (because I think "how do I build and deploy an app with Cesium" has a lot of low hanging fruit we can address once the initial ES6 is done).

@mramato
Copy link
Contributor Author

mramato commented Sep 24, 2019

@kring I know you've mostly migrated to TypeScript in your work, but if you have any thoughts about what I've been doing with ES6 here, please let me know.

@markerikson
Copy link
Contributor

What would the difference be between shipping Source and shipping Build/CesiumES6?

A major potential benefit of a proper ES6 module implementation would be tree shaking. I should be able to import just specific parts of Cesium, and have the rest left out of my final app bundle. Shoving everything into an "ES6 roll-up build" seems like it would likely make that very difficult. If the entire codebase is ES6 modules already, pointing the default entry point to Source/Cesium.js may be all that's needed. Unfortunately, the topics of tree shaking, pre-transpiling package artifacts before publishing, dealing with varying potential usage environments, and module formats, is an incredibly complex topic that I still only partly grasp myself.

I do agree that shrinking the published NPM package would be very nice. The Cesium NPM tarball is currently around 25MB, which is gigantic relatively to other packages.

@mramato
Copy link
Contributor Author

mramato commented Sep 24, 2019

If the entire codebase is ES6 modules already, pointing the default entry point to Source/Cesium.js may be all that's needed

That's a good point, that's actually what I'm doing in CesiumViewer and then setting treeshake.moduleSideEffects: false in rollup to eliminate unused modules.

I assumed the same setting would work if I built a combined ES6 version of rollup, but I just tried it and it's not smart enough to eliminate the unused code in that case. So for code size and tree-shaking, we actually don't want to combine at all.

So the answer is to ship some form of the Source directory (maybe renamed and streamlined) and the code would end up looking like:

import {
    ArcGISTiledElevationTerrainProvider,
    Math as CesiumMath,
    Viewer
} from './Source/Cesium.js';

Or whatever the directory gets named.

I'm not sure the npm tarball will shrink much, if at all. But the release zip we put up on GitHub and the website might. The main goal is ES6, but there's definitely room for a separate task to look at file sizes and what else we're including in npm that may not need to be there.

@mramato
Copy link
Contributor Author

mramato commented Sep 24, 2019

I'm not sure the npm tarball will shrink much, if at all.

Actually, this is a separate bug. We appear to be shipping the Apps folder as part of the build and that shouldn't be the case. Pretty sure we'll cut off 9 megs just from that.

I was wrong, master is correct and clocks in at 25MB. However the ES6 port should shrink this by 9MB because our workers now share code.

@TJKoury
Copy link
Contributor

TJKoury commented Sep 24, 2019

What @markerikson said. It needs to be separate modules for tree-shaking to work correctly.

@mramato
Copy link
Contributor Author

mramato commented Sep 25, 2019

So the es6-at-last branch has everything except running tests against the built Cesium release (However, I had a eureka moment on addressing that and will have a fix up later tonight).

Assuming all goes well have the final PR open for review by tomorrow (though we'll hold off on merging until post-Oct 1 release).

For anyone that wants to check it out in the mean time, either sync up that branch or just grab this release zip or point to this npm package

Any and all feedback is appreciated.

@kring
Copy link
Member

kring commented Sep 25, 2019

Thanks @mramato, I'm in the process of trying it out with Terria now and will let you know how it goes. All looks good so far though.

@mramato
Copy link
Contributor Author

mramato commented Sep 25, 2019

Awesome, thanks. I just pushed fixes for running our tests against built Cesium (rollup is awesome) so I think I'm actually "finished". I'm going to open a new PR with a full summary of everything I did so people can actually review the code.

I also have another list of ideas for future clean-up, stuff that's not ES6 related directly, but things we can now do because of it.

@TJKoury
Copy link
Contributor

TJKoury commented Sep 25, 2019

@mramato Looking good so far over here. What is the purpose of the WorkersES6 folder? Future-proofing for when workers are able to import?

@TJKoury
Copy link
Contributor

TJKoury commented Sep 25, 2019

@mramato Might I also recommend you create a HelloWorld es6 version with:

<script type="module" crossorigin="use-credentials">
     import * as Cesium from '../Source/Cesium.js';
     let viewer = new Cesium.Viewer('cesiumContainer');
</script>`

@mramato
Copy link
Contributor Author

mramato commented Sep 25, 2019

@mramato Looking good so far over here. What is the purpose of the WorkersES6 folder? Future-proofing for when workers are able to import?

WorkersES6 is the actual source code for our workers. Porting them to ES6 was needed so that they can import and use modules from Core/Scene/etc.. The build step compiles WorkersES6 down to ES5 AMD modules into the "old" Source/Workers directory. That's what actually get used/loaded by Cesium's worker system. It also makes it trivial to use ES6 in workers in the future, but not the main reason we did it.

@TJKoury
Copy link
Contributor

TJKoury commented Sep 25, 2019

@mramato Makes sense.

@mramato
Copy link
Contributor Author

mramato commented Sep 26, 2019

Might I also recommend you create a HelloWorld es6 version with:

I definitely think we want a basic ES6 example somewhere, but I'm probably going to get the main PR into master first and then have a separate PR to clean up packaging/examples/etc.. I may even create a separate repository for rollup similar to what we have for webpack (which will need to be updated as well.

@TJKoury
Copy link
Contributor

TJKoury commented Sep 26, 2019

@mramato I think now that you have everything as es6 modules, you shouldn't need separate repos for each build system.

In any case, you did a great job, this stuff looks amazing and certainly makes life much easier!

@mramato
Copy link
Contributor Author

mramato commented Sep 26, 2019

I think now that you have everything as es6 modules, you shouldn't need separate repos for each build system.

Agreed, but I just want to make sure we have good doc for best practices when working with Cesium, might just be a blog post or tutorial instead of a separate repository. Because you need to copy Assets/Workers/etc.. and often set CESIUM_BASE_URL, a lot of users struggle the first time they go through the process and I feel like we've never done a good enough job documenting it.

@mramato mramato mentioned this issue Sep 26, 2019
@mramato
Copy link
Contributor Author

mramato commented Sep 26, 2019

#8224 is open!

@markerikson
Copy link
Contributor

Great work. Looking forward to seeing the results!

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

No branches or pull requests

4 participants