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

Get Cesium Viewer working using ES6 #8193

Closed
wants to merge 25 commits into from
Closed

Get Cesium Viewer working using ES6 #8193

wants to merge 25 commits into from

Conversation

mramato
Copy link
Contributor

@mramato mramato commented Sep 19, 2019

One of my first goals for ES6 was simply getting Cesium Viewer working as an ES6 application. Which proved easier than I thought.

  1. Update GLSL shader gulp task to generate ES6 modules instead of AMD.

  2. Update Cesium.js build task to generate ES6 Source/Cesium.js instead of AMD

  3. Add rollup as a new dependency that will ultimately replace requirejs.

  4. Create a simplified createWorkers task that runs as part of build this generates one AMD module per-worker that is then loaded by TaskProcessor just as before. This step is way faster than I expected and also extracts common code into separate bundles, reducing overall file size and load time. We will make this more robust when we go to build the release/minified workers, right now I just care about running locally as a dev. I had to modify TastProcessor._defaultWorkerModulePrefix to point to Workers/Build instead of Workers, we'll have to change this somehow when we do a release build.

  5. Updated our mostly manual shims on Source/ThirdParty files to use ES6 instead of requirejs.

  6. I'm using import.meta.url in buildModuleUrl, which may not be the
    ideal solution but gets the job down for now. It's only a stage 3 proposal
    so eslint fails to aprt it. Open to any ideas on an alternate approach
    here (or should ES6 apps just always set CESIUM_BASE_URL before including
    anything)

  7. Also updated eslint to treat most of our code base as ES6, there are
    still some errors but nothing that won't go away on its own as we progress.

One of my first goals for ES6 was simply getting Cesium Viewer working as
an ES6 application. Which proved easier than I thought.

1. Update GLSL shader gulp task to generate ES6 modules instead of AMD.

2. Update Cesium.js build task to generate ES6 Source/Cesium.js instead of AMD

3. Add rollup as a new dependency that will ultimately replace requirejs.

4. Create a simplified `createWorkers` task that runs as part of `build`
this generates one AMD module per-worker that is then loaded by
`TaskProcessor` just as before. This step is way faster than I expected
and also extracts common code into separate bundles, reducing overall file
size and load time.  We will make this more robust when we go to build
the release/minified workers, right now I just care about running locally
as a dev. I had to modify `TastProcessor._defaultWorkerModulePrefix` to
point to `Workers/Build` instead of `Workers`, we'll have to change this
somehow when we do a release build.

5. Updated our mostly manual shims on Source/ThirdParty files to use ES6
instead of requirejs.

6. I'm using `import.meta.url` in buildModuleUrl, which may not be the
ideal solution but gets the job down for now. It's only a stage 3 proposal
so eslint fails to aprt it.  Open to any ideas on an alternate approach
here (or should ES6 apps just always set CESIUM_BASE_URL before including
anything)

7. Also updated eslint to treat most of our code base as ES6, there are
still some errors but nothing that won't go away on its own as we progress.
@cesium-concierge
Copy link

Thanks for the pull request @mramato!

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

@mramato
Copy link
Contributor Author

mramato commented Sep 19, 2019

@shunter obviously I would love to get your eyes on this if you have the time, but if you're busy with other stuff, that's fine.

Who else wants to review? We're all learning here so consider the review process part of taking the time to understand the new architecture and learn more about ES6.

shehzan10 and others added 3 commits September 20, 2019 10:05
For MVP ES6 support, we're going to run in ES6-aware browsers only, so
no IE.  Chrome and Firefox works great (and I suspect Safari as well).

We can definitely look into transpiling back to ES5 in a future update, but
I'm more worried about overall ES6 support right now.

This loses the ability to run tests against the release build, but I plan
on hopefully putting that back in before everything merges up to master.

There are a couple of test failures, but those will get looked into
separately than the actual test running mechanism itself.

1. Runs via node/CI and in the browser (which I honestly didn't think we
could keep going).

2. Switch to `karma-coverage-istanbul-instrumenter` for instrumentation
since we need it for native es6 support

3. Generate an ES6 version of SpecList.js and update spec-main.js to use
it.

4. Change to karma-conf.js to treat specs as ES6

5. Fix up some paths/specs to actually work.
@hpinkos
Copy link
Contributor

hpinkos commented Sep 20, 2019

Edge and IE don't seem to be working. Is that expected?

@hpinkos
Copy link
Contributor

hpinkos commented Sep 20, 2019

Should buildApps work?

@hpinkos
Copy link
Contributor

hpinkos commented Sep 20, 2019

@hpinkos
Copy link
Contributor

hpinkos commented Sep 20, 2019

Code changes look fine to me though

@mramato
Copy link
Contributor Author

mramato commented Sep 20, 2019

Talking with @hpinkos offline, to move faster we're going to switch to a coarse 2 PR approach. Basically #8192 will stay open until the end (as planned) but everything else will go into this right away. This way you don't have the 16k lines of cruft getting in the way of the review, but reviewers can just do on PR review at the end.

Get ES6 unit tests and coverage running
# Conflicts:
#	Apps/CesiumViewer/CesiumViewerStartup.js
#	Specs/karma-main.js
#	Specs/spec-main.js
#	package.json
1. Switch to rollup for combining/minifying JS code.
2. Switch to clean-css for combining/minifying CSS code.
3. Get eslint passing
4. Switch to `esm` for Node.JS ES6 support and make Node.js work again,
which required re-fixing third-party code shims and upgrading AutoLinker
and topojson
5. Remove `generateStubs` which is no longer being used.
6. Fix some merge issues with missing ES6 module imports
7. Deleted unneeded `Source/main.js`
8. Add back VERSION property on Cesium object.
Remove requirejs and update packaging for ES6
* Remove Cesium usage from Sandcastle proper, since it's not really needed
* Generate a VERSION propertyin the gallery index since Cesium is no
longer being included.
* Remove requirejs from Sandcastle bucket
* Update bucket to use the built version of Cesium if it is available
by fallbackto the ES6 version during development.
* Minor Cesium ES6 fixes found during development.
Move Worker code into `WorkersES6` and build Workers into the `Workers`
directory. This is so we (and users) don't have to do string manipulation
to rename `Workers/Build` -> `Workers` when building ES6 based apps that
rely on source modules.
Workers -> WorkersES6, Workers/Build -> Workers
@mramato
Copy link
Contributor Author

mramato commented Sep 26, 2019

Closing this to open a new PR with full summary and ready for review!

@mramato mramato closed this Sep 26, 2019
@mramato mramato deleted the es6-viewer branch October 17, 2019 00:34
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