ES6 Modernization: when to update old usage patterns?

[ptrgags]

As CesiumJS relaxes ES5-only syntax (see https://github.com/CesiumGS/cesium/pull/10396) and eventually allows more modern features, the question is what to do about the existing code, when does it make sense to update old patterns to use newer features?

To take a concrete example, consider the AssociativeArray class. This is a class that's designed to store key-value pairs and allow iteration. This sounds very much like an ES6 Map in terms of what operations it supports. Though this leads to questions like:

• Should we remove AssociativeArray and replace it with Map? (after considering overall performance impact (not microbenchmarks) and how much it impacts the public API).
• If we need a backwards-compatible interface, should the AssociativeArray just store a Map and delegate functions?

Another example: There are spots in the code where a dictionary is used like a set (need a collection of keys with O(1) lookup time):

• In CustomShader, I used a dictionary with dummy values for storing shader variables (e.g. {key1: true, value: true}), though a Set would have had a clearer intent that I just need it for fast key lookups. (This might be a bad example, as set union/intersection would be helpful as well for comparing sets of shader variables more like a Python set)
• In ModelOutlineLoader, the edges are stored in a strange data structure: an array used like a dictionary except index 0 stored the original vertex count... The ModelExperimental replacement code for this feature will use the dictionary approach (which can be easily swapped out for a Set)
• Where else? We'd have to search through the code to see what other parts of the code fake a set data structure using an AssociativeArray, a dictionary, or something else.

[jjhembd]

Here's another pattern that appears frequently in our code:

for (const key in myObject) {
  if (myObject.hasOwnProperty(key)) {
    const value = myObject[key];
    // do something with value
  }
}

This pattern loops unnecessarily over inherited properties.

We could now use the ES8 method Object.values, which only returns 'own' properties:

for (const value of Object.values(myObject)) {
  // do something with value
}

If the property names are also needed, we can use Object.entries with a destructuring assignment:

for (const [key, value] of Object.entries(myObject)) {
  // do something with key and value
}

Or alternatively,

Object.entries(myObject).forEach([key, value] => {
  // do something with key and value
});

[jjhembd]

A few more old polyfills that could be replaced:

• requestAnimationFrame: replace with the Web API method
• cancelAnimationFrame: replace with the Web API method
• combine: replace with Object.assign. Most cases could be directly replaced, but a few using the deep flag would require more thought
• getAbsoluteUri, getBaseUri, getFilenameFromUri, and a few others: These are mostly wrappers around the third-party library urijs, which is now deprecated in favor of the Web API URL

These are all on the public API, so we would need to phase them out with a deprecation period.

[ptrgags]

Also ES6's arrow function expressions (() => {}) are nice for anonymous functions. They use different scope rules so you don't have to do the const that = this; pattern. Example:

before:

SomeClass.prototype.method = function() {
  // inside a method
  const that = this;
  promise.then(function() {
    // have to use that because this refers to the anonymous function, not the SomeClass instance
    that.doSomething();
  });
}

after:

SomeClass.prototype.method = function() {
  promise.then(() => {
    // this refers to the instance of SomeClass, i.e. the behavior we want.
    this.doSomething();
  });
}

[ggetz]

We've begun using many of the features described in this issue, such that I don't think keeping the issue open is helpful. We'll discuss additional feature usage as they come up, and update any automating linting rules or the coding guide when appropriate.