Update eslint rules #1665
Merged
Update eslint rules #1665
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
victorlin
commented
Apr 22, 2023
tsibley
reviewed
Apr 24, 2023
victorlin
force-pushed
the
victorlin/update-eslint-rules
branch
from
1ff15ad to
c2fcdea
Compare
|
@jameshadfield let me know if you have specific code-style rules that you've found beneficial. We can consider (re-)adding them in this PR, or they could come in a separate PR. |
joverlee521
previously approved these changes
Apr 25, 2023
victorlin
force-pushed
the
victorlin/update-eslint-rules
branch
from
c2fcdea to
d4abbfd
Compare
Currently, the ESLint configuration file is top-level but only files under src/ are linted according to the npm script. This is a bit disjoint. Two options: 1. Move all usage of ESLint to src/. 2. Work towards applying ESLint to all files in this project. I'm going with (2) since there are some unlinted files that could benefit from linting. The first step, done by this commit, is to update the npm script and specify the unlinted files explicitly in .eslintignore (also removing unused ESLint exceptions from these files). Future commits will continue in this direction.
This was added in 55f4842 for context of where the initial rules came from. The rules have since deviated from the linked configuration file.
The extension-less configuration file name was deprecated more than 5 years ago¹, and the deprecation message is no longer present in the official documentation. Even though the extension-less file still works fine, the rename serves to keep up-to-date with documented formats and make it clear to some text editors that YAML is the format used in this file. ¹ https://dev.to/ohbarye/eslintrc-without-file-extension-is-deprecated-3ngg
victorlin
force-pushed
the
victorlin/update-eslint-rules
branch
from
d4abbfd to
550dde8
Compare
victorlin
force-pushed
the
victorlin/update-eslint-rules
branch
from
550dde8 to
50c7d7d
Compare
|
Latest rework LGTM by inspection. |
tsibley
approved these changes
May 3, 2023
jameshadfield
requested changes
May 4, 2023
There was a problem hiding this comment.
Thanks @victorlin for this! It's hard to track exactly what's changed, but given the ad-hoc nature of our previous config I'm not too worried about this (and one could argue it was hard to know what was set in the first place!). All the rules we had turned "off' (e.g. these react rules) were presumably enabled via the air-bnb preset (have verified a couple of these, but the air-bnb config is 1000s of lines of JS 😬). I spent most of my review looking at the rules we had previously enabled ourselves, and which have all been removed here. Most of them are pretty marginal so I don't mind them being removed by this PR, but there's a bunch I'd like to keep. At one time or another each has been explicitly added to our config, so there is a rational for them (even if we perhaps didn't document it as well as we could have).
There is just one, which can be wrapped in parentheses to make it valid. Rule doc page: https://eslint.org/docs/latest/rules/no-cond-assign
The recommendation to use `Object.prototype.hasOwnProperty.call()` is already used elsewhere in this codebase, so I assume these deviations were due to unintentional oversight. Rule doc page: https://eslint.org/docs/latest/rules/no-prototype-builtins
Since the existing unused variables have somewhat meaningful names, add underscores to these names and configure the rule to allow underscore-prefixed names to be unused. This commit does not address unused imports and functions - those are temporarily disabled by line and will be addressed in following commits.
node-fetch is no longer necessary as a direct dependency. Move it to devDependencies since it is still used in a test.
Keep the direct dependency since it is still used in src/util/parseMarkdown.js.
The only usage was removed in a7bfd62.
There are better ways to address these (componentWillMount¹, componentWillReceiveProps²). I opted for the rename to avoid complex code changes. ¹ https://legacy.reactjs.org/docs/react-component.html#unsafe_componentwillmount ² https://legacy.reactjs.org/docs/react-component.html#unsafe_componentwillreceiveprops
There is only one, so disable it for the specific line. A proper fix will come in the following commit.
Previously when missing, it defaulted to "latest" and showed a warning.
victorlin
force-pushed
the
victorlin/update-eslint-rules
branch
from
4adfc9b to
902895a
Compare
No code style rules yet but they will be added in subsequent commits.
This takes advantage of configuration cascading¹ so rules that only apply within src aren't applied elsewhere. ¹ https://eslint.org/docs/latest/use/configure/configuration-files#cascading-and-hierarchy
James went through all the rules that we had custom-enabled prior to b3cedb7. These are the ones that were flagged to be re-added. Some are commented out since there are existing violations. Subsequent commits will un-comment the rules and address violations. Co-authored-by: James Hadfield <[email protected]>
There is only one, and it is because a function expression is used before it is defined. Changing to a function declaration solves this because those are hoisted to the top-level (function expressions are not). Rule doc page: https://eslint.org/docs/latest/rules/no-use-before-define
As the rule doc page says, if a variable is never reassigned, using the const declaration is better. Rule doc page: https://eslint.org/docs/latest/rules/prefer-const
This was already disabled from 910e567 and it looks like there's enough context to keep it as-is. Rule doc page: https://eslint.org/docs/latest/rules/no-unneeded-ternary
These seem fine to have, so disable the rule per-line. Rule doc page: https://eslint.org/docs/latest/rules/no-console
The last reference was removed in 5d63682. This doesn't show up as "unused" during linting since it's possible that downstream code could import this function, but I don't think we have to worry about that for this specific function.
This is enabled in the Airbnb base config¹ that we previously used. It's still useful to have around. It can be disabled for the violation in title.js since using indexes as keys is only discouraged if the order of items may change² (not applicable here). ¹ https://github.com/airbnb/javascript/blob/7982931ba745c0f57ba6934fc7e6cc43901dac76/packages/eslint-config-airbnb/rules/react.js#L388 ² https://legacy.reactjs.org/docs/lists-and-keys.html#keys
victorlin
commented
May 4, 2023
There was a problem hiding this comment.
@jameshadfield thanks for the going through the removed custom rules and picking out what's useful – I've added everything that you requested. There is still a gap in checking the rules previously enabled by Airbnb config for anything that's now disabled but might be useful to add back (such as react/no-array-index-key), but as you mentioned, it's a lot to go through and not something I want to do at the moment.
|
For future reference, this is the Airbnb base config that was extended prior to b3cedb7: |
This was referenced May 5, 2023
Merged
7 tasks
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.
Description of proposed changes
Replaced Airbnb base config with other "recommended" base configs, and made other adjustments around that change.
Related issue(s)
Testing
console.logstatements within src/components/map.js'sMap.UNSAFE_*methods, ran dev server locally, clicked around successfully, verified presence of console outputs to indicate that the methods were invoked.(to be done by a Nextstrain team member) Create preview PRs on downstream repositories.I don't think this is necessary but feel free to create one if you'd like for reviewing.