Update Coding Guide with notes on destructuring assignment

[jjhembd]

As flagged by @javagl in https://github.com/CesiumGS/cesium/pull/12188#pullrequestreview-2298455422: The current Coding Guide does not discuss destructuring assignment.

Since https://github.com/CesiumGS/cesium/issues/10486, many ES6 patterns have naturally found their way into the code, including destructuring assignments. For example,

const {
  option1,
  option2,
  option3 = 3.0,
} = options;

This syntax is often both more concise and easier to read, compared to the legacy pattern:

const option1 = options.option1;
const option2 = options.option2;
const option3 = defaultValue(options.option3, 3.0);

This raises 2 questions for the Coding Guide:

• Should we recommend destructuring where appropriate?
• How do we clarify where destructuring defaults can be used, and where it is necessary to use defaultValue? The destructuring default syntax will replace undefined with the given default, but will pass null through.

[jjspace]

Personally I'm a fan of destructuring everywhere it makes sense. I don't think it's main benefit is default values though. I'll also mention https://github.com/CesiumGS/cesium/issues/11674 just to crosslink related issues.

[ggetz]

Rather than putting this in the coding guide, this should be enforced through eslint: https://eslint.org/docs/latest/rules/prefer-destructuring

[ggetz]

Are there any performance implications to using destructuring assignment everwhere?

[javagl]

Regarding the handling of null, compared to defaultValue: There are very few places where null is used at all. So that may not be critical.

Regarding the performance: There are some claims, but ... these are soooo 2018 (i.e. it's hard to measure that objectively across browsers and JITs).

Regarding the eslint: For now, this was rather asking whether this should become part of the coding guide. Iff it should become the recommended way, then yes: Everything that can be covered with a linter should not be mentioned in the coding guide. (Except, maybe, caveats like the null stuff, TBD).

But... highly subjective: I'm not sure about enforcing that particular rule. Destructuring assignment can be convenient when you really just have an object with 10 properties and want to pull out these 10 properties as local variables. But in some cases, the syntax is really obscure. For example, consider something like

let bar;
...
bar = object.bar;

Now, that innocent, bread-and-butter, self-explanatory last line would be flagged by eslint. It should actually be written as ({ bar } = object); (Yeah, really... 🤪 )

[jjspace]

Now, that innocent, bread-and-butter, self-explanatory last line would be flagged by eslint. It should actually be written as ({ bar } = object); (Yeah, really... 🤪 )

I was going to call out that specific example too, that looks awful to me. I think there's merit for at least a mention of destructuring in the coding guide as a suggestion "where it makes sense to do so" but I'm not sure I like the way eslint may choose to enforce it. Most of the time I think it's a subjective thing on a case by case basis of whatever makes the code more readable.

On performance, I have never seen anything mentioned that one is better or worse. I'd assume with how long it's been around most browsers should be able to optimize around both pretty well. That said I haven't done any research into it so I could be wrong.

[javagl]

For purely syntactical changes (like the destructuring), I'm leaning towards applying it once throughout the codebase. (Compared to other "cleanups", like using class or async/await, which may have to be done more incrementally).

If we want to generally prefer desctucturing assignment, then I could imagine that one viable approach could be to enable the linting rule and fix all places that if flags (except for the ones that recommend this obscure ({ bar } = object); syntax. (And... maybe this can even be disabled in eslint?)).

There certainly are still cases that are not captured by the linting rule. For example, code like

const batchTableJson = parsedContent.batchTableJson;
const batchTableBinary = parsedContent.batchTableBinary;
if (!defined(batchTableJson) || defined(batchTableBinary)) {
  return;
}

would be flagged to be converted into

const { batchTableJson, batchTableBinary } = parsedContent;
if (!defined(batchTableJson) || defined(batchTableBinary)) {
  return;
}

But the "equivalent" code

const{ batchTableJson = parsedContent.batchTableJson;
if (!defined(batchTableJson)) {
  return;
}
const{ batchTableJson = parsedContent.batchTableJson;
if (!defined(batchTableJson)) {
  return;
}

would not be detected as such a case.