A32nx: [META] Tips and tricks to keep JS running fast

[devsnek]

Don't reassign function parameters

function foo(bar) {
  bar = 1; // no!
}

Don't add or remove properties from objects.

delete obj.x; // no!

const obj = { y: 1 };
obj.x = 2; // no!

class X {
  constructor() {
    this.y = 1;
  }

  method() {
    this.x = 2; // no!
  }
}

Use specific methods over generic loops where possible

// no!
for (const item of array) {
  // ...
}

// yes!
array.forEach((item) => {
});

Don't use eval

Just don't.

Don't create arrays with holes.

// no!
const array = new Array(5).fill(0);
// yes!
let array = Array.from({ length: 5 }, () => 0);

Use strict equality

// no!
a == b;
// yes!
a === b;

---

Maybe more to come

[davidwalschots]

Potential eslint rules

• Don't reassign function parameters: no-param-reassign.
• Don't add or remove properties from objects.: for delete could consider using no-delete of eslint-plugin-fp.
• Use specific methods over generic loops where possible: 🤷‍♂️
• Don't use eval: no-eval.
• Don't create arrays with holes: while not capturing your example, no-sparse-arrays prevents one of these cases.
• Use strict equality: eqeqeq.

Given the following warning in .eslintrc.json I decided not to create a PR:

// DO NOT CHANGE THIS FILE (unless you are an admin) (if admin, make sure you change the same file in the latest asobo branch as well)

We might want to consider setting these up as warnings to start with?

[Benjozork]

I'll see if I can get those rules applied.

[beheh]

@devsnek What's the reasoning for

Use specific methods over generic loops where possible

Array.forEach lacks any way to break out of the loop, forcing the code to run for every element even if e.g. a sequence of specific elements is found early. Regarding pure performance I've found sources that argue both ways, so I think for a realistic, performance-based assesment an in-app performance test would be the way to prove this without a doubt.

But I would argue that unless the loop is extremely simple, the choice of for...of vs forEach is negligible compared to the actual code inside the loop. In that case, if there is any chance to break early a for...of loop is probably preferable. A linting rule to enforce forEach therefore seems very debatable.

As a side note, in many cases there are native methods that are more suitable than a loop. These depend on the specific use-case and could be referenced in a style guide. These are methods like:

• Array.every, Array.some
• Array.find, Array.findIndex, Array.lastIndexOf

These are mostly implemented optimally, where they exit early once a match has been found. There's also Array.map, Array.reduce and Array.reduceRight which, while there is no immediate performance benefit, can save the engineer from having to write boilerplate code and accidentally introducing suboptimal code.

[devsnek]

Like I said, using specific methods (every/some/filter/etc) is preferred, and you can still use loops if you need to represent some sort of specific control flow (I did say "where possible")

[beheh]

Sure, but then why even defer it to users to mix the styles here rather than choosing the more flexible pattern right away?
Especially if people are considering ESLint rules to warn contributors (many of whom might not very proficient in JS due to the nature of this project) it seems like it might be confusing and a user may choose to follow the original suggestion rather than ignore it over a more favorable control flow. It's not hard to imagine a user going _"Oh, looks like break is simply not available in .forEach. I'll just let the loop run to completion, can't be that bad"_.

From the issue title I assume that this suggestion is based on knowledge about the performance benefit of .forEach, but I was wondering about this myself as I'm lacking experience there and can only find conflicting information in various blog posts about this. Is it such a performance benefit? And if it is, are we also sure this performance benefit holds in MSFS, and by such a margin that it's worth having especially new engineers make the nuanced choice every time, as you describe?

Finally, my comment about specific array methods was simply a suggestion to add a reference to the top post, as I'm imagining lots of new users might stumble upon the initial comment in this issue and would benefit from learning about .every/.some/etc. rather than blindly replacing their for loops with a .forEach.

[devsnek]

There is no magic key to JS performance. JS engines try to recognize patterns of logic and reduce them into faster versions of that logic. Different JS engines look for different patterns, and those patterns change over time. What was faster in Safari 5 years ago may actually be slower now. In this case, for used to be faster than forEach in JavaScriptCore, and now forEach is faster by around 45-50%. I benchmarked all of these suggestions in an instrument before posting them here, as I don't know the exact version of JavaScriptCore we are working with.

A good eslint rule for this (and I'm not entirely sure that there is such a rule anyway) would not suggest changing from a loop to a method if the logic couldn't be properly represented.

Also, I am not suggesting that we only use forEach. It was just an example. Generally, you should use whichever methods are closest to your intent.