Cc/linting ts

[ChristopherChudzicki]

What are the associated issues?

#2488

What does this PR do?

Enforces linting rules via ESLint instead of the StandardJS CLI. The ESLint configs used are

• standard for js
• standard + plugin:mocha/recommended for tests
• standard-with-typescript for typescript.

How should this be manually tested?

Check that linting still works as expected in js, and that linting now works in Typescript files (whereas it did not before).

Why is this using Chris' fork of eslint-config-standard-with-typescript

This currently uses my fork of https://github.com/standard/eslint-config-standard-with-typescript, which doesn't seem great, and if there's a desire to wait to merge this until that's an official version is used. ESLint 8 has been out for 6 months so seems like the thing to use; standard-with-typescript was updated (on github) to use ESLint 8 a few days ago, but the npm version has not been published yet. Hopefully soon Until then, this is using https://github.com/ChristopherChudzicki/eslint-config-standard-with-typescript/releases/tag/v22.0.0

Linting Errors

These shows the linting errors that were present after initial commit on thsi branch, if anyone is interested: https://drive.google.com/drive/folders/18FHojFNNwCGJFwAp3IUb8KjvUeL8ft9i?usp=sharing

[gwhitney]

Not sure it is connected with the failing tests, but this PR currently changes { ..., key: key, ...} to { ..., key, ...} which I am pretty sure fails on IE11. So at least until June 15, better tune it to allow key: key

[josdejong]

Thanks @ChristopherChudzicki for working this out! That is a huge amount of work!

I'm glad that the code style is mostly the same as before (all of the changes in #2541 would be bit too much indeed 😅). The only left over changes make sense to me: replacing { key: key } with { key }, and it's fine with me to replace arrow functions with regular functions in tests.

The tests fail because of an issue with the git url of eslint-config-standard-with-typescript. I would love to get that resolved. Links to a github repo as npm dependency is ok with me temporarily during development, but not acceptable for production. I'm a bit worried though whether eslint-config-standard-with-typescript will be released any time soon (last release was 8 months ago). What do you think, shall we just wait for a couple of days, and if not, come up with an alternative solution?

@gwhitney the { key: key } replacement will not be an issue: when creating an ES5 compatible bundle, this will be replaced again by the transpilation done by Babel. We should put that transpilation in place in typed-function too, so we can finally use modern JS with all these niceties like { key }.

[ChristopherChudzicki]

I'm a bit worried though whether eslint-config-standard-with-typescript will be released any time soon (last release was 8 months ago).

@josdejong I think that's definitely a concern. Even if they release a new npm version from current master within the next week or so, how well will it be maintained with future releases of the peer dependencies? And there are 5 peerDependencies with non-* versions. So I think this is a real concern.

In general, it seems to me that StandardJS's support for typescript is not great. The support is better when using StandardJS configs through ESLint rather than the StandrdJS CLI, but still not particularly good, as we're seeing with eslint-config-standard-with-typescript. (Aside: their CLI support still has a peer dependency of Typescript 3.. TS 4 was released 2 years ago.)

From #2488 (comment)

I do think we should have a single linting setup for the whole repo though, and not something different for the plain JS vs the TypeScript definitions. Would it make sense to completely replace StandardJS with ESLint+Prettier and configure it correctly (and consistent) for both JS and TS?

I think there are conflicting goals:

1. similar standards for TS files and JS files
2. well supported libraries
3. small diff

I think we can basically get any two but not all three: (1) and (2) mean dropping StandardJS, which implies a big diff, probably not too dissimilar from that in #2541.

But I think we could skirt (1) reasonably. @josdejong, what would you think of using an ESLint config along the lines of

// for js (and a variant with 'plugin:mocha/recommended' for js tests)
    extends: ['standard']
// for typescript
    extends: [
        'plugin:@typescript-eslint/recommended',
        'plugin:prettier/recommended'
    ]

This would be sort of separate setups for TS and JS. But

• Everything is enforced through ESLint. So:
  • Good IDE support (e.g., in VSCode, you'd just need the ESLint extension)
  • There's still just npm run lint and npm run lint -- --fix.
• Small diff, because source code and tests still following StandardJS
• Good maintence:
  • StandardJS seems reasonably maintained for JS files And the StandardJS CLI uses ESLint under the hood, so I imagine any updates to StandardJS have to go through their ESLint config, though I'm not 100% sure.
  • Prettier is definitely well maintained
• We can tweak Prettier's config a little bit. It's not very configurable, but we can tell it to use "no semicolons" and "single quotes", which keeps the TS formatting style fairly similar to the JS formatting style.

IMO, ☝️ is probably the sweet spot of effort / maintenance / git history. @josdejong What do you think?

[josdejong]

That makes a lot of sense, good idea. Right now we do not have TypeScript code in the project except for the type definitions. So I think it's ok if the TS code style is not exactly the same as the JS code style. Makes sense to take that approach as the first step.

In another future step we may want to move towards "eslint" defaults and away from "standardjs". But since that will be a huge change breaking all open PR's and branches, let's only do that if there is real need for it and then plan it smartly.

[ChristopherChudzicki]

Closing in favor of #2544