Use ES6 "class" as module syntax

[worlddai]

So I can extend your class directly using extends,and so on.

[OmarShehata]

This is to support browsers that don't yet support ES6 syntax without requiring a transpilation step. Here's a quote from our blog post going over the whole migration that I encourage you to check out:

Even though we’ve moved to ES6, the combined Cesium.js file still supports browsers that lack ES6 support, such as IE 11. As long as your build system can target IE 11, Cesium ES6 modules will continue to work without requiring additional transpilation.

With that said, having a transpilation is something we want to support in order to use newer ES6 syntax and features. Also from the blog:

Since Chrome, Firefox, and Safari already support some of the latest JavaScript features, we should be able to keep our minimal build during development policy in place and start to leverage newer features such as async/await/Promise/generators/etc… We would still need to run the final output through Babel as part of the build process, but Rollup already has good support for this step.

I'm going to leave this open as a feature request to use this particular ES6 feature, since even if we do add a transpilation step it would have to be another effort to change how classes are declared.

[mramato]

@OmarShehata I'm actually going to close this, the use of class will be part of a much larger discussion/decision/change so having this open as a standalone issue isn't really actionable.

[thw0rted]

@mramato is there a different (good) place to discuss this publicly? I'm currently consuming Cesium from Typescript (c.f. #5717 #4434 etc) and I think I could fully automate that process if the "class-like" new-able function pattern that Cesium uses were to move to ES2015 class syntax. I explain in the "Typings" issue, but briefly, Typescript can successfully extract type information from JSDoc for almost all of Cesium, except where getter/setter functions are passed to Object.defineProperties and decorated with @memberof.

As a small test case, I converted Viewer.js by hand, which took about 5 minutes of manual copy/pasting. Most of the work was converting the getter/setter syntax from foo: { get: function(){...}, set: function(...) { ... } } to get foo() {...}; set foo(...) { ... };. I haven't looked at automating this yet but I believe I can at least make the manual process pretty straightforward. After making this conversion, the Typescript output (.d.ts files) is correct, and the test suite still passes.

[ggetz]

I think it's valid to have this issue open:

1. We no longer support IE 11
2. This would be a discrete step towards full TypeScript

We will need to discuss the strategy for how to port over code.

If we'd like to take a similar approach to how we moved to using prettier, we'd need some sort of automated method. I just came across Lebab, the opposite of Babel, for converting from ES5 syntax to ES6+. We'd need to do a test run to see if its possible to use for our codebase.

[javagl]

Some brainstorming in terms of the strategy (even though some of that may be obvious):

Two important dimensions are

1. which part of the code will be updated
2. which updates will be applied

---

For 1, some options are:

• Just update the code as you see fit, while editing it. E.g. either updating a certain class while your're refactoring it anyhow, or casually throwing in some promise-to-await change when fixing some bug in some function.
  • The drawbacks here are that this could be highly cluttered. The codebase would be a pretty inconsistent mix of different styles for a long time. We already have that to some extent (e.g. Pomises vs. await). But this could make it more apparent, more widespread, and (most importantly): It would never be "finished" (whatever "finished" means here). First, because new ECMAScript versions are created each year. And secondly, because some parts of the code are never touched in practice.
• Update groups of classes in one pass
  • This could refer to "packages" like Core or Scene, or subsets thereof. For example, a relatively low-hanging fruit could be to start with all the math-related CartesianX/MatrixX/Ray/Polygon... classes. This could make sense because it's very basic stuff, and could be "rewarding" in many ways (also a step towards that sought-for Cesium math library). A counterargument could be: These are exactly the classes that are never touched, and they already are relatively clean and easy to understand. There are other areas of the code that could benefit more from all this.
• The "big bang" - update everything at once.
  • This is the approach that was taken with the 'prettier' transition. But it would be far more difficult here. The 'prettier' changes did not so much change the structure of the code itself, but mainly its ... "style" (i.e. the changes had small, local impact, like "var -> const" or "prefer template over concatenation" and so).

---

For 2. the options are ... well, at least everything that is listed as "Transforms" on https://github.com/lebab/lebab , and more...

(Note: The considerations here are independent of whether we give 'lebab' a try or not. It seems unlikely for me that this could be a one-shot solution in general. And even if it was, technically, the following still has to be kept in mind: )

The possible modernization changes vary wildly in terms of their impact and their potential for "breaking" something. One recommendation from the lebap README is

The recommended way of using Lebab is to apply one transform at a time, read what exactly the transform does and what are its limitations, apply it for your code and inspect the diff carefully.

and this also applies if we did certain changes manually.

---

Note that these are really independent dimensions. For example, we could

• Change Promises to async/await only in DataSources, or everywhere at once
• Change prototype to class everywhere, or only in the math-related classes
• ... (all combinations thereof...)

---

I think that 2., "Which updates will be applied", can be answered more generically. A few random examples of the changes that could be part of such a modernization (roughly inspired by the lebab README):

• Changing prototype to class? Yes!!!
• Changing array.indexOf(foo) !== -1 to array.includes(foo): Yeah... why not?
• Changing Math.pow() to **? Ouch - I didn't even know that this existed. That's a "nope" for me...
• Changing for (let i=0; i<array.length; i++) { const item = array[i]; }-loops to for (const item of array)? I think that this has far worse performance, so probably not (or not everywhere...)
• (not in the lebab README): Changing promise chains to async/await? Yes!!!
• ...

Now... we can argue. Or vote 🙂

[jjspace]

I'll start by saying I fully support swapping to class simply for the better support from TS and Intellisense which should make working on the library better and as mentioned before could be an easier midway step toward #4434

Just update the code as you see fit, while editing it

Even if this seems like the smallest effort up front I think of all the options this is the only one we should definitely not do. I think this approach can easily turn every PR into a mini-refactor and make them take longer. It also obfuscates the actual change of a PR. Much easier to review 1 pr for some change and a second PR that is only code restructure but should behave identically before and after.

I think going section by section could work pretty well and unless we can fully automate transformation through something like lebab then that's probably the only feasible approach. I do think we should try and have a concerted effort to move through the whole codebase (or at least package level engine/widgets) fairly quickly to avoid messy merges or out of sync branches.

which updates will be applied

I think we should focus this issue and the subsequent PRs/changes on only the transition from prototype based classes to the keyword class classes.

Unrelated to your 2 points we may want to just take a peek at performance. I believe there were articles years ago when class was a new "syntax sugar" about how people should "not" use them because it obfuscates how the JS actually works and could be worse for performance. My assumption is that is no longer a concern and with newer class specific features like proper private properties or static and instance properties introduced in ES2022 I would expect that browsers and JS engines have gotten a lot better at handling class classes.

Some random articles I found doing a quick google

• Making Sense of ES6 Class Confusion | Toptal® - 6 years old
• Please stop using classes in JavaScript - everyday.codes - 2020
• Stop Using JavaScript Classes! - 2022, I don't think the module approach is good (no inheritance etc) but pointing it out as a more recent article
• popular alternative that's a bit more functional oriented instead of OOP are factory functions Class vs Factory Function: Exploring the Way Forward - Bomberbot

[javagl]

Even if this seems like the smallest effort up front I think of all the options this is the only one we should definitely not do. I think this approach can easily turn every PR into a mini-refactor and make them take longer. It also obfuscates the actual change of a PR. Much easier to review 1 pr for some change and a second PR that is only code restructure but should behave identically before and after.

Although it probably does not make sense to try and talk through all degrees of freedom, scenarios, and granularities here, I think it's important to be aware that there are many degrees for this. The point is: There is no clear cut between "applying style and conventions" and "a refactoring".

For example, when someone is touching a piece of code, and, say, adds another option to an options structure, which is later extracted via the common

const option1 = options.option1;
const option2 = options.option2;
const thatNewOption = options.thatNewOption;

then one could make a case to just update this, to

const {
  option1,
  option2,
  thatNewOption,
} = options;

iff (if an only if) the coding guidelines suggest that this is the preferred way (see #12196 ).

Similarly, when someone is fixing a bug where some wrong argument is passed to a function as in

loadData().then(function(data) {
    process(data, "wrongArgument");
});

one could make a case to not just change the "wrongArgument", but change this to

const data = await loadData();
process(data, "rightArgument");

However, this is related to the question of which changes are (supposed to be) applied.

And when there is one, specific change, like the "prototype-to-class", then I agree that this should be addressed holistically, and (important:) in isolation. Later, we could do another holistic change, also in isolation (like "then-to-await" or so).

[javagl]

A specific detail: Right now, it is never clear which functions are actually implementations of a certain "interface". In some cases, the JSDoc mentions something like /** This is part of the "Example" interface */ or so. Properly "extending parent classes" or "implementing abstract base classes/interfaces" is a clumsy with prototype, and more streamlined with class. And one thing that I'd really like to see being used more consistently is the @inheritdoc and/or @override annotations.

[jjspace]

Should we even be considering this

I wanted to do some due diligence to make sure there were not going to be any performance implecations with this change. In general I have not found anything that would indicate this is a bad change or would negatively impact our performance overall. If you have seen anything like that please let me know, happy to discuss.

class is just syntactic sugar around the way we define classes currently so the way browsers treat them should be fairly similar. Any pitfalls we would fall into using class are likely also possible using prototypes. However they're probably less likely with class definitions because the structure and tooling that's built around it should help catch things earlier. They've also been supported in all major browsers since 2016 with the "newer" class features like private fields since 2020-2022. My intuition says browsers should have them pretty optimized by now.

MDN's in depth dive on Inheritance and the Prototype chain makes a couple mentions of performance but mostly focused on the fact that the way we currently do it is more error prone even if there is potentially a small benefit. I think it would be good to switch to class and get all the advantages of that from the DX perspective and selectively convert things back only when we notice them performing poorly.

Another side note that I find compelling is that Three.js uses classes Mesh, Light, Scene. If it's good enough for them I question why it wouldn't be good enough for us

All of this is also a completely moot point when considering that a large percentage of users will probably have some sort of build tooling of their own that will re-transform our code and may or may not keep the class structure. Also, as I understand it, once we switch to TS that will likely generate the prototype versions for us so any performance benefit that has will be done the "proper" way from the TS compiler.

_{Couple other links to interesting reads but not directly relevant composition, hidden classes}

Path forward

Speaking with @ggetz I think we came up with a pretty good plan to move forward with this that allows us to trial the change and prove out the process without a full refactor of the entire library

1. Break out /Core methods and classes into a new @cesium/core package
   • This is something we've wanted to do for a while anyway Publish smaller packages #10636
   • This also creates a better defined "hard boundary" for a reduced set of CesiumJS's code to focus on without getting into semantics of the directory structure
   • This new package could be used only internally at first if we're not ready to expose it fully, tbd
2. Refactor @cesium/core to use class instead of prototypal classes
   • With it being a smaller package it should be less work overall and I believe many of the classes in Core are already some of our smaller ones so progress should move quicker.
3. Convert the type generation of @cesium/core to tsc for Update or remove tsd-jsdoc #10455
   • The work for this issue currently seems needed to support the transition to better type generation
   • Again I believe that being focused on just this new package will allow us to prove out that change in a more focused environment

[jjspace]

Small status update with some general findings

• I created the branch core-package to transfer the core functions/classes to a new package. It has very basic build steps set up but the imports are not all updated.
• I created the branch core-class-refactor off that branch to start the class conversion and figure out the process. There are a handful of classes already converted and some adjustments that were needed to make sure docs still generate as they currently do.

Some observations/notes on the process

• lebab does work well for converting prototype classes to class classes but it does not support defineProperties which means every class will need a small manual step to convert those properties.
  • support for Object.defineProperties() lebab/lebab#267
  • The conversion is a fairly simple replacement from a pure text perspective and I threw together a very quick converter from Object.defineProperties to pure getter/setter syntax in this jsfiddle which makes it a 2 step process to copy -> paste, copy -> paste
  • I think it could be possible to automate this with an extra transformer using esprima or something to actually manipulate the AST but I didn't want to spend too much time digging into that
  • It was actually fairly straightforward to extend lebab to include support for this. I've created a fork and opened a PR for that Add support for defineProperties and static class properties lebab/lebab#365. Even if it doesn't get merged we can probably use my branch. I tested it quickly on a few classes and it seemed to be working well
• JSDoc issues/changes
  • The doc comment for our constructors gets added to the class itself but should be moved to the constructor to match the current documentation. A future discussion could be had about how to document classes vs constructors
  • The @memberof tag should be able to be safely dropped everywhere once things are inside a class. If it's not it seems to confuse JSDoc and it moves/removes properties
  • The @readonly tag should stay on the necessary properties. JSDoc does not seem to automatically detect a property that only has a getter
  • When using a getter and setter only one should be documented for the generated documentation. If both are documented it duplicates it in the generated docs ES6 getter or setter, how to define them? jsdoc/jsdoc#973.
    • I think the getter makes more sense
    • We will have to check how this plays with TS and intellisense but I assume being on the getter should be good
  • static properties are not getting documented as static even with the @static tag
    • class static property don't work jsdoc/jsdoc#2044
      • I found a "workaround" for this which can be seen in the refactor branch by hooking our plugin into the doc generation and forcing JSDoc to recognize these as static. Feels a little hacky but it does work, see the docs for Ellipsoid
      • Alternatively through some testing it seems that leaving them outside the class block lets JSDoc still recognize them as static automatically but I'm not sure how well that lines up with the "correct" way to define classes
    • A similar issue may affect inheritance too but I haven't run into it yet: Documentation of static method of sub-class is not inherited jsdoc/jsdoc#1874

CC @ggetz

[javagl]

I have not yet read through all the changes in the branches that you linked to (they are huge, as expected, and identifying the actual changes will probably be easier by looking at the commits, and not the changes as a whole).

But one thing that seems to be pretty relevant in this prototype-to-class transition: As explained in the "Private Functions" section of the Coding Guide, a common pattern is to not declare private functions as "members", but instead, create a file-scoped function and pass this as the first parameter. I think that this is ... not a good idea. The whole idea of "classes" and "private" is to have one, cohesive "thing", starting at the { and ending at the }. I think that such functions should be converted into proper "member" functions (as usual, with the options of doing that 1. when editing anyhow, 2, in one package (like Core), or 3. in a huge, concerted PR).

[jjspace]

Follow up RE: lebab and defineProperties

I took a little time over the weekend to take a stab at adding support for defineProperties and it was actually easier than I expected. I now have a fork (and a PR) for this support. I tested it on a few of our classes and it seemed to work well. Definitely should remove one more transformation step in this process.

[MichaelK-Bertrandt]

A specific detail: Right now, it is never clear which functions are actually implementations of a certain "interface". In some cases, the JSDoc mentions something like /** This is part of the "Example" interface */ or so. Properly "extending parent classes" or "implementing abstract base classes/interfaces" is a clumsy with prototype, and more streamlined with class. And one thing that I'd really like to see being used more consistently is the @inheritdoc and/or @override annotations.

I am looking into using cesium for a project and this point is important to me.
Figuring out what i can pass as DataSource without a documented hierarchy is diffcult. I can guess that GeoJsonDataSource is one, but what about e.g. DataSourceCollection?

So from an explorability viewpoint this change would be huge.