style: prefer arrow functions

[turadg]

Description

Our coding style is to use arrow functions but much code remains from before that decision.

https://github.com/JamieMason/eslint-plugin-prefer-arrow-functions provides a dandy way to automatically fix over the repo.

Before merging:

• decide on https://github.com/JamieMason/eslint-plugin-prefer-arrow-functions#classpropertiesallowed
• decide on https://github.com/JamieMason/eslint-plugin-prefer-arrow-functions#disallowprototype
• decide on https://github.com/JamieMason/eslint-plugin-prefer-arrow-functions#returnstyle
• decide on https://github.com/JamieMason/eslint-plugin-prefer-arrow-functions#singlereturnonly
• decide on whether to include arrowParens in Prettier (we punted at the last upgrade because of the churn, but these are all about to be churned)

I propose this as the path to transforming the whole codebase

• make this PR be responsible for the eslint config
• to demonstrate different configs, spin out branches that have those configurations (currently on this PR, so would pull them off
• once we agree on the lint config, merge it using this PR
• one package at a time do the lint-fix (though this is a little more complicated if we couple a prettier change)

Security Considerations

-->

Documentation Considerations

Testing Considerations

[dckc]

solo is not a global. It's defined in this file.

Somehow the conversion to arrow functions changed export default async function solo() { ... } to an anonymous export default const async () => { ... }

[turadg]

Makes much more sense. Seems like a bug in the transformer, not considering that a default export may have its name used locally. We can work around it by defining const solo = and export default solo.

[mhofman]

We can work around it by defining const solo = and export default solo.

That would make me sad as a pattern.

[turadg]

We don't want that! Any alternative solutions you would recommend?

[erights]

decide on https://github.com/JamieMason/eslint-plugin-prefer-arrow-functions#classpropertiesallowed

The playground at https://astexplorer.net/#/gist/7c36fe8c604945df27df210cf79dcc3c/12f01bed4dcf08f32a85f72db0851440b7e45cdd shows that classPropertiesAllowed: true rewrites

class MyClass {
  render(a, b) {
    console.log(3);
  }
}

to

class MyClass {
  render = (a, b) => {
    console.log(3);
  };
}

This is not semantics preserving, or even close. Therefore, let's decide false.

That is the setting in the PR, so not requesting any changes for this.

[erights]

decide on https://github.com/JamieMason/eslint-plugin-prefer-arrow-functions#disallowprototype

I can't figure out what is this, from either the description or the playground. First, do no harm. Let's decide on false to minimize unintended damage.

That's current setting in this PR, so no change requested.

[erights]

decide on https://github.com/JamieMason/eslint-plugin-prefer-arrow-functions#returnstyle

The current setting on this PR is "unchanged". Both @dtribble and I would prefer "implicit". But a brief experiment showed it rewriting

const coerceLR = (h, leftAmount, rightAmount) => {
  // @ts-ignore cast (ignore b/c erroring in CI but not my IDE)
  return [h.doCoerce(leftAmount.value), h.doCoerce(rightAmount.value)];
};

to

const coerceLR = (h, leftAmount, rightAmount) => [
  h.doCoerce(leftAmount.value),
  h.doCoerce(rightAmount.value),
];

which is not semantics preserving, because we put some semantics in comments. But even if we didn't, deleting user-written comments is unacceptable.

Let's decide to keep this PR's current setting of "unchanged".

Not requesting any changes for this.

[erights]

decide on https://github.com/JamieMason/eslint-plugin-prefer-arrow-functions#singlereturnonly

This one should obviously be false, as it is in this PR. No change requested.

[erights]

I corrected arrowParents in the top comment to arrowParens.

[erights]

decide on whether to include arrowParens in Prettier (we punted at the last upgrade because of the churn, but these are all about to be churned)

We actually argued this one out long ago and decided on the current behavior: that a parameter list containing only a single identifier must not have parens. I didn't inject this in the recent churn because we arrived at the same conclusion anyway.

It seems this is the meaning of the "avoid" setting, which is the current setting of this PR. No change requested.

[erights]

once we agree on the lint config, merge it using this PR

I propose we have agreement, pending only more eyes on the actual code to see if there is any unexpected damage with these settings. We still need to do some of that before we can approve and merge.

[erights]

Just doing a yarn lint at the top of a fresh checkout of this PR, I found much damage. I commented on the first, in bundle.js . This seems like a show stopping degree of damage. I'm marking this "request changes" to block merging. But I expect the changes we need is to the rewriter itself. Otherwise we should just close this PR but leave corresponding bugs open.

[erights]

WTF It removed the terminating $' from the literal string. If this transformation is this buggy, I don't think we can use it.

[turadg]

transformation too buggy to use