Update to eslint flat config and yea/nay all rules #3240
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
reosarevok
force-pushed
the
eslint-9
branch
7 times, most recently
from
2f41204
to
e71d8e7
Compare
This is the default for eslint 9 and will be the only option in eslint 10. See https://eslint.org/docs/latest/use/configure/migration-guide There's apparently a bug with the globals release we had automatically installed and eslint flat config, so just updated to the latest version manually since that one is fixed.
For some reason (.mjs differences?) these two files had eslint issues that were not reported with the old config.
Rather than having a convoluted system to check unfixed rules, this just has them apply to all files, and only selectively disables them for files we know to be unfixed. This helps us not introduce more issues in the meantime. I did generalize a bit for release-editor and similar folders where we don't expect to fix the files but convert them entirely, so there's no point fixing them file by file.
For some reason (.mjs differences?) these files had eslint issues that were not reported with the old config.
This is a new recommended rule in eslint 9: https://eslint.org/docs/latest/use/migrate-to-9.0.0
Most stylistic rules have been split from the eslint package and moved to a separate one - the eslint ones still work but are officially deprecated, so we might as well update before they break. I'll move these to a separate block in a second commit to make it easier to review.
https://eslint.style/rules/js/dot-location We consistently use the property dot location position. Especially given how that's not even the eslint default, it seems worth enforcing with a rule.
https://eslint.style/rules/js/generator-star-spacing We consistently use the 'after' style. Especially given how that's not even the eslint default, it seems worth enforcing with a rule.
This was listed under errors, but it's a stylistic rule.
This was listed under best practices, but it's a stylistic rule.
This was listed under best practices, but it's a stylistic rule.
This was listed under ECMAScript 6, but it's a stylistic rule.
This was listed under ECMAScript 6, but it's a stylistic rule.
https://eslint.style/rules/js/yield-star-spacing We were inconsistent here, but I talked to mwiencek and we both prefer the 'after' default.
Most stylistic React rules have been split from the react package and moved to a separate one - the react ones still work but are officially deprecated, so we might as well update before they break. I'll move these to a separate block in a second commit to make it easier to review.
https://eslint.style/rules/jsx/jsx-curly-newline We were inconsistent here, but not horribly so. Auto --fix'ed 38 issues.
The old rule is deprecated in 9.0.0 and replaced with the new one, which is already available, so making the change in preparation. We don't have any clashes with the new rule, it seems.
reosarevok
changed the title
|
@brainzbot, retest this please |
https://eslint.org/docs/latest/rules/prefer-spread GuessCase/Main was messy enough that this refactors it a bit to make Flow less angry.
https://eslint.org/docs/latest/rules/require-await This mostly hit cases where we didn't actually need async since they return Promises. There was one case where we needed to wrap some returns in Promise wrappers, but that's about it.
We don't need this to remind us to use strict, but it did warn me of one unnecessary use of it! So worth having on for that.
https://eslint.org/docs/latest/rules/yoda Three cases of this we had, a bit stupid it looked.
This adds some rules that seem useful as warn, and lists all others as off, so we know we looked at them and decided against them rather than not having decided yet.
https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/no-extraneous-dependencies.md We only had one of this, defined in selenium.mjs... which seems to be pretty useless anyway, so just replacing it with a simpler option (we shouldn't really care if the message is '' or undefined).
https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/prefer-default-export.md There's no good reason anymore to not have a default export for manifest (we used to have two exports but we no longer do). Other files were a lot more trivial. tableColumns is clearly meant as something that might eventually have more exports, and so is report/constants, so I disabled the rule there.
This activates a few more rules and specifies the ones we do not want are intentionally off. There's a few rules that do not currently work with a flat config setup but we probably do want; I commented those out for now.
This activates a few more rules and specifies the ones we do not want are intentionally off.
This activates a few more rules and specifies the ones we do not want are intentionally off.
This activates a few more rules and specifies the ones we do not want are intentionally off.
https://github.com/flow-typed/eslint-plugin-ft-flow?tab=readme-ov-file#enforce-suppression-code Our Multiselect typing is a horrible mess that we will fix in a separate PR, for now this disables the rule there since we have to ignore *four* Flow rules for them.
https://github.com/flow-typed/eslint-plugin-ft-flow?tab=readme-ov-file#no-internal-flow-type We were using these to avoid importing React all the time, but a recent Flow version made it so that it's no longer needed, so we might as well return to the standard types.
This activates a few more rules and specifies the ones we do not want are intentionally off.
This activates a few more rules and specifies the ones we do not want are intentionally off.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
eslint 9 is now out: https://eslint.org/blog/2024/04/eslint-v9.0.0-released/
There's a new configuration default, "flat config" (https://eslint.org/docs/latest/use/configure/configuration-files) - this is supported in eslint 8, default in eslint 9, and will be the only option in eslint 10. The first commits here move our config to use this system. While doing this I changed the way we deal with unfixed rules, which now are considered as fixed and enforced globally while disabling them only where they are known to still fail. This should help avoid introducing new code that doesn't follow the rules.
Stylistic rules are deprecated in the main eslint package because they have been moved to
@stylistic/eslint-plugin-js, so this moves them to use the new package.Actually moving to eslint 9 cannot happen at least until
eslint-plugin-reactfixes its compatibility (unless we want to just comment out all their rules temporarily, but I don't think that'd be too smart), but at least we can migrate the config, which only conflicts with a couple rules that need updating (and are commented out for now in the cases where we will probably want them later).Further commits look through all the rules from the plugins we use, implement all that seem useful, and explicitly turn off all others, to make it clear that we have indeed looked into them and do not want them.