Types: Convert Cartesian2-4.js to ES6 classes

[donmccurdy]

Description

Converts Cartesian2, Cartesian3, and Cartesian3 to ES6 classes. The changes are very easy to make semi-automatically (see steps below) but not entirely trivial to review. So I'm inclined to do a few "small" PRs like this first and build up confidence, then attempt bigger batches.

Steps

1. Grep for unwanted new Cartesian+\.[a-z], and remove 'new' in each case.
2. Grep to confirm CartesianN has no base or child classes. If it does, they need to be converted at the same time.
3. Using the script from Types: Bulk conversion to ES6 classes #13125, run node ./lebab-batch.js "packages/engine/Source/Core/Cartesian*.js". If the script outputs warnings, decide whether to skip these files or to manually resolve them, considering (1) above. In this PR, no errors were encountered.

Reviewing

Review is much easier with GitHub's split view, and whitespace changes hidden (?diff=split&w=1). Lebab shouldn't change the order of class members unnecessarily.

Issue number and link

• Use ES6 "class" as module syntax #8359

Testing plan

Unit test coverage and ESLint should give us pretty good confidence on a mechanical change like this — but if there are other specific concerns let's take a look. On a visual check the API documentation looks identical.

Author checklist

• I have submitted a Contributor License Agreement
• I have added my name to CONTRIBUTORS.md
• I have updated CHANGES.md with a short summary of my change
• I have added or updated unit tests to ensure consistent code coverage
• I have updated the inline documentation, and included code examples where relevant
• I have performed a self-review of my code

PR Dependency Tree

• PR Types: Convert Cartesian2-4.js to ES6 classes #13168 👈

This tree was auto-generated by Charcoal

[github-actions]

Thank you for the pull request, @donmccurdy!

✅ We can confirm we have a CLA on file for you.

[Hiwen]

Personally, I would recommend using a script to uniformly convert all js files, just like converting from AMD to ES6 syntax before.

I used Babel to parse the expression tree of JavaScript, then tried to convert it into an ES6 class. Currently, most of the conversion work has been completed, but there are still some issues that need to be addressed when running it.

[donmccurdy]

Hi @Hiwen! This PR was generated with the script from #13125. Internally my script uses lebab and espree. So far the script seems to do well — it fails for a few files (not in this PR) but at least alerts us to the files it cannot safely convert, so we can handle them manually.

Would you be interested in opening a PR or two helping to convert some remaining source files in packages/engine/Source/** 😇? Other conversion methods are definitely welcome, too. Preferably such PRs would have clear diffs (see comment above) and be small enough to review in an hour.

[javagl]

Grep to confirm CartesianN has no base or child classes. If it does, they need to be converted at the same time.

This suggests a certain "rigurousness"(?) of the language. There are no structures. There are no contracts. There is no meaning. JavaScript receives its meaning by execution. So maybe, in some ways, JavaScript is the origin of the phrase "It is what it is", because "duck typing" is one aspect of all that. And now there are adventurous things like

cesium/packages/engine/Source/Scene/ClippingPlane.js

Line 126 in becdc1f

function UpdateChangedCartesian3(normal, clippingPlane) {

, and I think that this could be considered to be a "child class" of Cartesian3 - at least, it is declared to be a Cartesian3 in the normal getter. I won't argue. I'm just wondering.

[Hiwen]

Hi @Hiwen! This PR was generated with the script from #13125. Internally my script uses lebab and espree. So far the script seems to do well — it fails for a few files (not in this PR) but at least alerts us to the files it cannot safely convert, so we can handle them manually.

Would you be interested in opening a PR or two helping to convert some remaining source files in packages/engine/Source/** 😇? Other conversion methods are definitely welcome, too. Preferably such PRs would have clear diffs (see comment above) and be small enough to review in an hour.

@donmccurdy Thank you very much for your invitation. I still need to do some research to further verify the feasibility of batch conversion. However, if batch conversion is carried out, it will be a huge PR, and it will be very difficult to complete the review within an hour!

[donmccurdy]

@javagl yes, that's fair. I'm aiming to provide a rule that's clear and legible enough to help us make progress safely and stay well clear of more time-consuming issues. Duck-typing is not a problem. But mixing ES6 class syntax with prototypal inheritance can be a problem, if a child class needs to invoke the constructor of a parent class, and only the parent is using ES6 class syntax. This came up for three.js in mrdoob/three.js#17425, with symptoms as shown in this JSFiddle: https://jsfiddle.net/w2xfb6eg/. Not a hard-and-fast rule, but will reduce the number of things we need to worry about and test, if we can avoid mixed the two in the same inheritance chain.

@Hiwen thank you! Please note #13125 — it contains a bulk conversion of most of packages/engine/Source/Core, with a script given, and passes unit tests. The difficulty is the code review ... we're not prepared to review and merge this as a single large PR. Small PRs on related groups of files would be best, I think we can review and merge those quickly.

[jjspace]

Thanks @donmccurdy, these changes look good to me. I did do a quick check of the docs and generated TS files and those look correct too.

Did you want to turn on type checking for these files in this PR? It seems like it should work well with the new classes and that was the thing that spurred this on correct?

[donmccurdy]

Hm, I'm not sure what the @alias tag's original purpose was but I can't see a difference without it after the ES6 Class conversion. I think we probably want to move the @param tags but put the description on the class rather than the constructor, a small change in documentation rendering?

Left: PR, in green. Right: Production, in red.

I've added // @ts-check as well since it's "mostly" straightforward in this case. There are some discrepancies in what the JSDoc types allow and what the method bodies implement, which I've resolved avoiding runtime changes. If those JSDoc changes require more in-depth discussion then I'd probably prefer to remove // @ts-check and go ahead with ES6 Class conversion without it. From a documentation rendering perspective, tsd-jsdoc seems to understand them:

Allowing Float64Array and Float32Array but not other typed arrays is entirely arbitrary, but my hunch is that this can/should be loosened to allow any typed array type in the future. I didn't look, yet, into how to represent a union of all typed array types non-verbosely in a way that both TS and tsd-jsdoc would understand.

[jjspace]

Allowing Float64Array and Float32Array but not other typed arrays is entirely arbitrary, but my hunch is that this can/should be loosened to allow any typed array type in the future. I didn't look, yet, into how to represent a union of all typed array types non-verbosely in a way that both TS and tsd-jsdoc would understand

We actually just recently ran into this limitation in another PR. It's really annoying that TypedArray doesn't actually exist so JSDoc won't accept it. #12963 (comment) If you can figure out a way to do it that'd be awesome for the future, for now I'm fine with leaving it as just the 2 types it seems we already use it with

Thanks for the changes @donmccurdy

[donmccurdy]

Thanks @jjspace! Attempting the TypedArray alias here:

• Types: Add 'TypedArray' and 'TypedArrayConstructor' type unions #13187

[Hiwen]

@Hiwen thank you! Please note #13125 — it contains a bulk conversion of most of packages/engine/Source/Core, with a script given, and passes unit tests. The difficulty is the code review ... we're not prepared to review and merge this as a single large PR. Small PRs on related groups of files would be best, I think we can review and merge those quickly.

@donmccurdy I tested your script and I think it works really well. Sorry I didn't notice it before.
I am very happy to participate in this process.