Replace when.js with native promises #10149

ggetz · 2022-02-28T21:20:44Z

Replaces #8525
Fixes #3967
Fixes #9578

This PR removes when.js in favor of native promises. Besides removing the files related to when itself, this generally follows the migration guide as laid out in the migration guide, swapping out when.defer with a new defer function, switching otherwise for catch, and always for finally.

There were a few areas in Source where a function was assumed to be executed synchronously when a function was resolved. Native Promises however, by spec, will resolve at the end of a frame. There were also some strange handling of promise rejection in imagery providers that I cleaned up a bit in order to make them testable.

In LabelCollection and EntityCluster, order of execution adjustment where made.
Added TextureAtlas.addImageSync to support the order of execution fix in LabelCollection
ArcGisMapServerImageryProvider.readyPromise will not reject if there is a failure unless the request cannot be retried.
SingleTileImageryProvider.readyPromise will not reject if there is a failure unless the request cannot be retried.

The majority of the changes lie in the Specs, where unresolved promises weren’t being awaited before finishing executions, and where resolved promises are assumed to be synchronous all over the place and needed a good amount of fixes. Another issue which came up was calling Promise.reject in the body of a spec can cause node to halt execution when running via the command line.

Finally, I did need to tweak eslint in order to turn off the rule which disallows promises.

~~Opening this a bit early. Still to do:~~

Merge master
Fix remaining spec failures, travis-only failures
Open issues for excluded specs, and for refactoring out use of defer in general
Add documentation for defer
Update CHANGES.md - Add note about ArcGisImageServerProvider rejecting ready promise if there is a failure and retry is false.
Check build-ts warnings

cesium-concierge · 2022-02-28T21:20:49Z

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.

chris-cooper · 2022-03-02T00:53:46Z

It will be great to see this at last! Did you consider a major version update due to the API change @ggetz ? https://semver.org/

ggetz · 2022-03-03T14:57:30Z

Thanks @chris-cooper, historically cesiumjs has chosen not to use semver due to frequent breaking changes especially early in development. However, we are presently considering moving to semver to make updates like this easier. We're in the process of confirming this decision since it will likely have impacts among government users. So we're thinking about it, but I can't promise a version increment for this next release.

ggetz · 2022-03-04T14:39:43Z

I know @kring mentioned that defer is not the preferred way to go, and to use the Promise constructor instead as its better practice for handling errors. I think for the sake of limiting the scope of this PR, we leave it in. However, I'm assuming we should at least add a warning to the defer docs explaining that's its preferable to use Promise directly instead?

mramato · 2022-03-04T18:24:32Z

I'm assuming we should at least add a warning to the defer docs explaining that's its preferable to use Promise directly instead?

This is private, right? So I assume you mean the internal documentation?

I agree with @kring that all of Cesium's defer usage would be better served via new Promise. I'm also okay with making that a second pass. It will be a lot easier to do than the effort this took, either way. We should avoid adding any new defer calls in future code.

ggetz · 2022-03-04T18:53:00Z

@mramato when.defer was part of the public API, so I assumed we should make defer public as well, at least for this initial change. If you recommend making a clean break, I can update the migration guide and suggest any usage of when.defer should instead be replaced with a new Promise.

mramato · 2022-03-04T19:25:59Z

@ggetz I would definitely recommend a clean break. Anyone that was using defer is going to have to update their code regardless.

ggetz · 2022-03-04T20:18:26Z

The docs, CHANGES.md, and the migration guide on the forum have been updated accordingly.

I'm still fixes a handful of specs, but this is almost complete and should be ready for review. @sanjeetsuhag Could you please do a pass on this?

sanjeetsuhag · 2022-03-08T17:15:21Z

@ggetz I went through all the Sandcastles and it seems like the only problematic one is CZML Custom Properties.

ggetz · 2022-03-08T19:52:15Z

Thanks @sanjeetsuhag, fixed! That test just needed to await loading the CZML.

The tests are also in order now. I had to exclude a handful, mostly 3D tiles related, specs as there was some questionable error handling that led to errors being thrown that cannot be sensibly caught. I'm opening an issue for these excluded tests and will link it back into the specs.

Specs/Scene/ModelSpec.js

ggetz · 2022-03-10T16:09:04Z

I wasn't able to duplicate locally, but I believed I've handled the intermittent failure that was happening in CI. @sanjeetsuhag do you have any additional feedback?

sanjeetsuhag · 2022-03-10T16:19:02Z

@ggetz I looked at the latest commits, and I have no comments on the code changes. I can only speak to the excluded ModelExperimental specs and that #10178 should be a high priority fix before the release: lots of important tests in there.

ggetz · 2022-03-10T16:25:35Z

Thanks @sanjeetsuhag. I do agree that its high priority, and labeled it accordingly. There's also only two tests in ModelExperimentalSpec which have been excluded because of the error handling, not the whole suite.

Apps/Sandcastle/CesiumSandcastle.js

Source/.eslintrc.json

mramato · 2022-03-10T23:57:41Z

Have we ran any performance benchmarks against main to ensure that we haven't introduced a perf regression in any of the most common areas? (Like CWT + 3DTiles loading) or simple.czml animating on an empty globe.

Not sure if we have any "canned" perf tests ready to go, perhaps @lilleyse might have some ideas about what we have done for 3D Tiles testing.

Source/DataSources/EntityCluster.js

Source/Scene/Globe.js

mramato · 2022-03-11T00:15:48Z

Source/Scene/LabelCollection.js

  if (!defined(this._backgroundTextureAtlas)) {
    this._backgroundTextureAtlas = new TextureAtlas({
      context: context,
      initialSize: whitePixelSize,
    });
    backgroundBillboardCollection.textureAtlas = this._backgroundTextureAtlas;
-    addWhitePixelCanvas(this._backgroundTextureAtlas, this);


This looks like a bugfix in addition to Promise update? Is that true? If so, what bug does it fix and should we include that in CHANGES? (even if we have no issue number for it).

I noticed a few places where there are little changes like this that appear to be bug fixes of some kind. If we just figured out the code is wrong but don't know how/if a bug manifested from it, that's fine too, nothing to report. But I just wanted to point it out in case.

These were to address cases where the code assumed a promise was executing synchronously. When transitioning to native promises, even in cases where the promise immediately resolved, it wouldn't do so until the end of the frame. So the timing of the rest of the function is thrown off. I don't think these fix any existing bugs, just keep behavior working as before.

mramato · 2022-03-11T00:22:33Z

Source/Scene/TextureAtlas.js

@@ -353,6 +352,49 @@ function addImage(textureAtlas, image, index) {
  textureAtlas._guid = createGuid();
 }

+function getIndex(atlas, image) {


As a general rule, stuff like this should be called out in the PR description. I assume by looking at it we had to add some branch new code in order to provide a path for synchronous operations that would otherwise be async in our new "native" Promise world, but in a PR this big; it would be easy for a reviewer to miss. Also, if someone needs to use revision history to track where/why this code was added, it won't be obvious to them by reading the PR description (which it should be).

Even just a single bullet like "Added TextureAtlas.addImageSync to xxxx" would be extremely valuable to future devs.

This is especially true given the terseness of many of the commit messages in this PR (or perhaps we are squashing) but either way, a paper trail is important.

I planned on squashing commits with a more useful commit message. But understood, I'll call out these changes in the PR explicitly.

Source/Scene/Vector3DTilePolygons.js

Source/Scene/Vector3DTilePolylines.js

Source/Widgets/Viewer/Viewer.js

mramato · 2022-03-11T00:28:59Z

Two big things that jumped out at me:

Holy smokes do we abuse defer!
Moving to async/await (Which can happen organically/gradually) would be a huge improvement for readability and verbosity for our code base.

mramato · 2022-03-11T00:29:55Z

Just noticed this:

Is that because of test fixes or something else? (This is more out of curiosity than anything else).

mramato · 2022-03-11T00:31:17Z

Okay @ggetz I'll admit I kind of lost steam when I got to the specs, so I didn't review them thoroughly by any means. But this PR looks great and those minor comments were all I had. This is a major stepping stone for CesiumJS and an awesome way to start a true modernization effort.

ggetz · 2022-03-14T15:23:10Z

Is that because of test fixes or something else?

Yes, the extra code is definitely coming from spec modifications. There were tons of cases where we needed to add a few lines to await a promise before continuing.

ggetz · 2022-03-14T16:13:48Z

Have we ran any performance benchmarks against main to ensure that we haven't introduced a perf regression in any of the most common areas? (Like CWT + 3DTiles loading) or simple.czml animating on an empty globe.

I've run several 3D Tiles and CWT examples including various photogrammetry, point clouds, and OSM buildings. I'm seeing similar loading times using the Chome profiler on main and this branch. Likewise with simple.czml I'm seeing comparable framerates. @lilleyse If we have more exact tests for performance here, please let me know and I'd be happy to compare.

lilleyse · 2022-03-14T20:55:35Z

I usually just record how long it takes for tiles to load for a given view. Sometimes I'll also set up a flight path. Here are two of my go-to sandcastles:

Globe	3D Tiles
Sandcastle	Sandcastle

ggetz · 2022-03-15T14:53:04Z

Awesome thanks @lilleyse!

I'm seeing a comparative range of loading times with both this branch and main for both the terrain and tileset examples.

And thanks @mramato for all the feedback here! I believe I've addressed everything at this point.

Is there any remaining feedback or is this ready?

There were a few areas in Source where a function was assumed to be executed synchronously when a function was resolved. Native Promises however, by spec, will resolve at the end of a frame. There were also some strange handling of promise rejection in imagery providers that I cleaned up a bit in order to make them testable. - In LabelCollection and EntityCluster, order of execution adjustment where made. - Added TextureAtlas.addImageSync to support the order of execution fix in LabelCollection - ArcGisMapServerImageryProvider.readyPromise will not reject if there is a failure unless the request cannot be retried. - SingleTileImageryProvider.readyPromise will not reject if there is a failure unless the request cannot be retried. The majority of the changes lie in the Specs, where unresolved promises weren’t being awaited before finishing executions, and where resolved promises are assumed to be synchronous all over the place and needed a good amount of fixes. Another issue which came up was calling Promise.reject in the body of a spec can cause node to halt execution when running via the command line.

mramato · 2022-03-16T14:11:47Z

ggetz mentioned this pull request Feb 28, 2022

Replace when with native Promises #8525

Closed

6 tasks

ggetz self-assigned this Mar 2, 2022

ggetz mentioned this pull request Mar 9, 2022

Factor out "defer" usage for better error handling #10178

Closed

mramato reviewed Mar 10, 2022

View reviewed changes

Specs/Scene/ModelSpec.js Outdated Show resolved Hide resolved

ggetz force-pushed the when-to-native branch from 7eba54d to 37c9e17 Compare March 10, 2022 15:51

mramato reviewed Mar 10, 2022

View reviewed changes

Apps/Sandcastle/CesiumSandcastle.js Outdated Show resolved Hide resolved

mramato reviewed Mar 10, 2022

View reviewed changes

Source/.eslintrc.json Show resolved Hide resolved