search

CesiumGS / cesium

An open-source JavaScript library for world-class 3D globes and maps :earth_americas:
https://cesium.com/cesiumjs/
Apache License 2.0
12.92k stars 3.48k forks source link

Plan to switch to ES6 syntax #9718

Closed ebogo1 closed 2 years ago

ebogo1 commented 3 years ago

Now that IE support has been dropped in 1.84, we should make a plan to update our code. This issue can be a place to brainstorm ideas for these changes.

There are some things that need to happen before we can start making changes in CesiumJS:

Other things to think about:

ptrgags commented 3 years ago

I'm curious about the switch to let and const, is this going to be a change for new code moving forward, or are you translating all the old code to use let and const? (are there tools for this? sounds like a big task otherwise)

I'm also curious to see when we can use for ... of loops for objects instead of the rather verbose:

for (var key in obj) {
    if (obj.hasOwnProperty(key)) {
    }
}

And other ES6 features (e.g. spread syntax where right now we use shallow combine()... there's a lot of helpful things to consider here (of course, weighing this against any performance differences)

onsummer commented 3 years ago

I'm glad to see the official intention to update the syntax of the source code.

Using let and const avoids the problem of variable promotion in JavaScript:

if (isUnderground) { // <-  Error, `isUnderground` is not defined
  // do something
}

let isUnderground = false

if use var the above code will run well:

if (isUnderground) {
  // do something
}

var isUnderground = false

const maintains the immutability of variables:

import { Cartesian3 } from 'cesium'

const coords = new Cartesian3(100, 200, 0)
coords = 0 // Uncaught TypeError: Assignment to constant variable.

If you use var to make the code run normally, there is generally no need to change it to let and const.

For the for ... of mentioned by @ptrgags , I have seen in some evaluation reports that the performance of iterative access to the object's key and value is acceptable, but the performance of iterative access to the array may not be as good as that of for ( let i = 0; i <someLength; i++) {/* some code */ } and the Array.prototype.forEach method.

Modularization technology has been changed from requirejs to esmodule in version 1.63, which has done a good job.

I have used the vitejs packaging tool, and have used CesiumJS in the reactjs and vuejs frameworks. Adding some guidance documents combined with these front-end frameworks maybe a good idea.

The above are some of my simple views, I hope it will be useful to the official.

In addition, is it possible to extract the math module of CesiumJS (most of the codes is in the Source/Core directory) separately into an npm package? This is a relatively complete js math library I have seen so far.

ebogo1 commented 3 years ago

Related issue: https://github.com/CesiumGS/cesium/issues/9610

ptrgags commented 2 years ago

First of all, I just want to say I'm very excited to see that #10026 got merged yesterday! Already enjoying being able to use const/let in my code

I'm very curious about what's next, as there's many ES6+ features I wish I could be using. this site has a good overview of features, though it's only ES6, not the later versions (e.g. async/await is actually ES2017). Of course, I realize that some of these features (in particular loops) need to be considered with care in regards to performance.

Given what my team is working on (3D Tiles Next features and ModelExperimental), these are some ES6 (ES2015) syntax that I would find very helpful day-to-day for more readable code:

Small Improvements

These features would be nice to have, at least for new code.

// instead of this which I find myself doing quite often in ES5 return { property1: property1, property2: property2 }

* destructuring assignment

const [x, y, z] = packedPosition;

// instead of const x = packedPosition[0]; const y = packedPosition[1]; const z = packedPosition[2];



### Larger Questions:

* Upgrading old CesiumJS code is a massive task. Do we always need to update everything at once (like the `const/let` changes) or can we do it gradually (allow new syntax for new files, update old code in smaller chunks?). I can imagine the latter would make the codebase more fragmented style-wise, but it would at least allow better quality-of-life for new development.
* for-of loops is perhaps the feature I want the most (especially for iterating over dictionaries). However, if performance is an issue, then we should be careful here.
    * We should do some benchmarks ourselves to see how big of a difference it is
    * It's also important to make a distinction of where the loop appears. There's a big difference between one-time initialization code and code that executes every frame. Best practices should be documented in the Coding Guide, but they would not be easily enforceable by `eslint`.
* Are there any CesiumJS built-in functions we can phase out in favor of ES6+ syntax? (some of these are longer-term wishes)
  * `combine(b, a, deep=false)` could be written `{...a, ...b}`. 
  * Likewise `clone(a, deep=false)` could be written `{...a}`
  * (ES2020 only) `defaultValue(value, default)` could someday be written `value ?? default` (see [nullish coalescing operator on MDN](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Nullish_coalescing_operator)
  * (ES2020 only) [optional chaining](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining) would be nice to reduce the number of `defined()` checks. E.g. `const metadata = tilesetJson?.extensions.?3DTILES_metadata`;
  * anything else I'm forgetting?
* Switching from when -> native `Promise` would be nice (though there are some subtleties, see #8525). Even better would be `async/await`, but that's only available in ES2017+
* Longer term, would we be able to start using `class` syntax, at least for new classes? the syntax is much nicer (especially for getters/setters).
ggetz commented 2 years ago

As a part of this, we should revisit Cesium's default eslintrc configuration as a whole (and the shared eslint-cesium plugin in this repository) to remove the out of date and unneeded rules keeping us on es5 syntax.

sanjeetsuhag commented 2 years ago

Background

Proposed Changes

Move eslint-config-cesium to its own repository

  graph TD;
      eslint:recommended-->eslint-config-cesium:::cesium;
      eslint-config-cesium-->eslint-config-cesium/browser:::cesium;
      eslint-config-cesium-->eslint-config-cesium/node:::cesium;
  classDef cesium color:#6dabe4

Upgrading eslint configuration in CesiumJS

  graph TD;
      eslint-config-cesium/node.js:::ES6-->.eslintrc.json:::ES6;
      eslint-config-prettier:::ES6-->.eslintrc.json:::ES6;
      eslint-config-cesium/browser.js:::ES6-->Source/.eslintrc.json:::ES5;
      eslint-config-prettier-->Source/.eslintrc.json;
      eslint-plugin-es:::ES5-->Source/.eslintrc.json;
      Source/.eslintrc.json:::ES5-->Apps/.eslintrc.json;
      Apps/.eslintrc.json:::ES5-->Apps/TimelineDemo/.eslintrc.json:::ES5;
      Apps/.eslintrc.json-->Apps/Sandcastle/.eslintrc.json:::ES5;
      Source/.eslintrc.json-->Specs/.eslintrc.json:::ES5;
      Specs/.eslintrc.json-->Specs/TestWorkers/.eslintrc.json:::ES5;

classDef ES6 color:#3fb950
classDef ES5 color:#da3633

Exceptions

As of 1.93, we use disable eslint in our code for the following rules:

Note: It was easy enough to find these because in most cases, we specify the rule being broken. However, in other cases, we just use eslint-disable-line. We should enforce that all rules being broken be explicitly mentioned.

Current Rules

At the moment, the primary source of ES6 restrictions come from the eslint-plugin-es plugin. Specifically, from the use of the plugin:es/restrict-to-es5 configuration. From the source code of the module, we can see that this configuration is composed of the following plugins:

Since each of these is composed of many rules, it may be cumbersome to create a PR for each one. Therefore, I propose that we incrementally work through each of these plugins - with a PR for removing the restrictions for each one, eventually ending up with support for ECMAScript 2020, which is what we should set our ecmaVersion to.

Additionally, going through the ESLint documentation for environments, I noticed that the jasmine environment only adds global variables for version 1.3 and 2.0. There is a more up to date plugin called eslint-plugin-jasmine. I don't want to update for the sake of updating, but I think it may be worth investigating once the core upgrades are done.

Another step may be to use eslint-plugin-jsdoc to ensure consistency in our documentation.

Proposed Rules

I went through all the rules available in eslint-plugin-es and found some rules that I think we may want to impose:

Ideally, anything we exclude we exclude with good reason. I selected these on first impression and am hoping I can get more feedback on what we should allow or not.

Upgrade coding guide

ggetz commented 2 years ago

Inside CesiumJS, there are 6 configurations:

While I think its definitely possible to cut down on the number here, we will most likely still need a few variations based on the target environment. As part of this plan, can we recommend a new simplified "dependency tree"?

sanjeetsuhag commented 2 years ago

@ggetz Could you review the updated comment above? I've added my ideas for how we should proceed with these changes and compiled a list of rules we may want to impose.

ggetz commented 2 years ago

Before we get too deep into what rules we plan on adding, I would suggest we streamline our existing configs first. That would include turning off plugin:es/restrict-to-es5. I believe if we just include the es plugin, it will default to restricting to our target ecmaVersion, which should currently be 2015/es6. Then we can remove the rules we've explitely turned off for the es6 features we've allowed so far. We should also audit the Apps/Source/Specs configurations, and make sure they are using the same ecmaVersion and rules.

There is a more up to date plugin called eslint-plugin-jasmine. Another step may be to use eslint-plugin-jsdoc to ensure consistency in our documentation.

More linting types certainly isn't a bad idea, but is tangential to this effort. Let's come back to those after first streamlining our configuration. Perhaps open a separate issue.

ggetz commented 2 years ago

I believe any actionable items here have been completed moved to other issues.