chore: upgrade to eslint 9.x #1267
Conversation
Upgrades to eslint 9.x and simplifies the customisation of our config.
| @@ -7,6 +7,8 @@ import * as parse5 from '../../packages/parse5/dist/index.js'; | |||
| import { ParserStream as parse5Stream } from '../../packages/parse5-parser-stream/dist/index.js'; | |||
| import * as parse5Upstream from 'parse5'; | |||
|
|
|||
| /* eslint-disable no-console */ | |||
There was a problem hiding this comment.
I don’t see a good reason to put file based ignores after import (export?) and before global definitions. I’d say before the imports makes more sense?
There was a problem hiding this comment.
Sure I just threw it somewhere near the top
Happy to move it
| }, | ||
| }, | ||
| eslintConfigPrettier, | ||
| ]; |
There was a problem hiding this comment.
This looks like it is removing plugin:unicorn/recommended? Why?
There was a problem hiding this comment.
It is
Much of it is covered by the formatter or the recommended rule sets
It would be good to know what exactly we wanted it to catch. It's a very big plugin for not much benefit by the looks of it
We're basically enabling a large set of rules without any particular reason other than assuming it'll catch useful stuff. Id rather avoid the clutter and rely on a formatter and the more important core rules
There was a problem hiding this comment.
Re the first paragraph, much of ESlint can also be done by hand, yet we still use ESlint.
Re the second, that is very subjective, and I strongly disagree.
Removing lint rules is always silent. The project was formatted. So of course you don‘t notice that lint rules are useful.
Re the last paragraph, I even stronger disagree. Lint rules are very useful in my experience.
There was a problem hiding this comment.
The last paragraph was very far from suggesting lint rules are not useful. We pulled a very large package in without really knowing which rules we cared about other than the ones we manually enabled (i.e I don't think any of us ever knew what was in the recommended config)
Much of it is stylistic and should be owned by the formatter (and already is). But if there are other rules in there we think are useful, I can reintroduce it
The recommended configs have grown since and cover all the more important stuff.
| @@ -26,7 +26,7 @@ | |||
| "parse5": "^7.0.0" | |||
| }, | |||
| "scripts": { | |||
| "build:cjs": "tsc --module CommonJS --target ES6 --outDir dist/cjs && echo '{\"type\":\"commonjs\"}' > dist/cjs/package.json" | |||
| "build:cjs": "tsc --moduleResolution node10 --module CommonJS --target ES6 --outDir dist/cjs && echo '{\"type\":\"commonjs\"}' > dist/cjs/package.json" | |||
There was a problem hiding this comment.
'node10' (previously called 'node') for Node.js versions older than v10, which only support CommonJS require. You probably won’t need to use node10 in modern code.
a) why is such an old thing needed here,
b) what does it have to do with eslint?
There was a problem hiding this comment.
Typescript will refuse to build with node16 resolution and commonjs module
Our config was wrong all along but the older tsc just didn't complain
Presumably installing tseslint bumped it in the lock file
So either we do this or we fix cjs builds properly (which means we can't just run a build twice but need a separate config which doesn't have type: module or vice versa)
Not my choice to rely on an old option. But with how we currently build our cjs output, this is correct it seems
There was a problem hiding this comment.
I switched my packages to https://github.com/isaacs/tshy for dual CJS and ESM builds to work around this (and improve the source maps situation).
Upgrades to eslint 9.x and simplifies the customisation of our config.