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.93k stars 3.49k forks source link

Update Coding Guide with notes on destructuring assignment #12196

Open jjhembd opened 1 month ago

jjhembd commented 1 month ago

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:

jjspace commented 1 month ago

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 commented 1 month ago

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

ggetz commented 1 month ago

Are there any performance implications to using destructuring assignment everwhere?

javagl commented 1 month ago

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 commented 1 month ago

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.