chore: set up eslint-plugin-simple-import-sort and eslint-plugin-import
#2086
Conversation
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
I haven't looked into either this one or #2085 yet but I'm inferring there's a fair amount of overlap since @gabrocheleau referenced your list from #2073. Assuming these two PRs cover the same territory, can y'all consolidate? |
Indeed, they both overlap significantly but this one covers some additional rules, so I suggest we keep this one. I'll review. |
config/eslint.js
Outdated
| '@typescript-eslint/strict-boolean-expressions': ['error'], | ||
| 'simple-import-sort/imports': 'error', | ||
| 'simple-import-sort/exports': 'error', | ||
| 'import/default': 'error', | ||
| 'import/export': 'error', | ||
| 'import/exports-last': 'off', // TODO: set to `warn` for fixing and then `error` | ||
| 'import/extensions': 'off', | ||
| 'import/first': 'error', | ||
| 'import/group-exports': 'off', | ||
| 'import/namespace': 'error', | ||
| 'import/no-absolute-path': 'error', | ||
| 'import/no-anonymous-default-export': 'error', | ||
| 'import/no-cycle': 'off', // TODO: set to `warn` for fixing and then `error` | ||
| 'import/no-deprecated': 'off', // TODO: set to `warn` for fixing and then `error` | ||
| 'import/no-duplicates': 'error', | ||
| 'import/no-dynamic-require': 'off', | ||
| 'import/no-extraneous-dependencies': 'error', | ||
| 'import/no-mutable-exports': 'error', | ||
| 'import/no-namespace': 'off', | ||
| 'import/no-restricted-paths': 'error', | ||
| 'import/no-self-import': 'error', | ||
| 'import/no-unresolved': 'off', | ||
| 'import/no-unused-modules': 'error', | ||
| 'import/no-useless-path-segments': 'error', | ||
| 'import/no-webpack-loader-syntax': 'error', | ||
| 'import/order': 'error', |
There was a problem hiding this comment.
Do we actually need all those additional rules? My intuition would be to only include rules that currently result in an update when applied for performance and general simplicity. I haven't looked up what each of these rules does in detail but here's a couple examples of rules that I suspect don't result in changes:
'import/no-absolute-path': 'error',
'import/no-anonymous-default-export': 'error',
'import/no-restricted-paths': 'error',
'import/no-self-import': 'error',
Curious to hear your thoughts on that, and more generally, I personally feel like it'd be great if the additional rule was justified, so that we avoid bloating up our linting configuration.
There was a problem hiding this comment.
I generally treat linting as a preventative measure rather than applying rules when an issue pops up so I would say it depends on your stance on that. People use dozens of different editors and IDEs and each of them has their own ways of generating imports and exports and handling monorepos which makes rules like the absolute path and anonymous default export rules useful. The self-import rule is a rather odd rule but automatic refactoring in certain tools can generate code that triggers that rule.
Let me know if you want me to remove those rules and I'll push that.
There was a problem hiding this comment.
Yeah I can totally get behind the preventative approach, I guess my main concern is I'd like to make sure that the added rules are relevant, mostly because I don't currently have a clear view on why some of these rules were added to start with.
I think it's worth thinking about what our criteria for adding new rules should be. Some examples that I have doubts about:
- we are adding
'import/default'(which ensures that default imports are coupled with exports) but not'import/named'(which ensures that named imports are coupled with exports). group-exportsandexports-last, is this something we actually want? I tend to lean in favor, but it requires a non-trivial amount of work, and I wouldn't consider it a no-brainer.no-restricted-paths, from what I understand this is only useful in the context where "restricted zones" are defined, but I don't think we have those
I've ran into situations, on other repos, where an excessive amount of resource-intensive linting rules led to performance issues for some contributors, hence my preoccupation with ensuring that additional rules serve a useful purpose.
Tagging the rest of the team in case they'd like to voice an opinion on that. @holgerd77 @acolytec3 @jochem-brouwer @ScottyPoi @g11tech
There was a problem hiding this comment.
- Added the
import/namedrule - The
group-exportsandexports-lastrules I'll take care of in subsequent PRs. - Removed the
no-restricted-pathsrule
There was a problem hiding this comment.
I don't have past experience with complex linting setups beyond the monorepo so can't comment on performance, but I don't notice any differences between running the linter on master vs this PR on my VSCode setup.
As far as individual rules in this set go, the only one we'll have to revisit is the import/extensions if/when we add ESM support. Otherwise, this list looks good to me and I like having all the imports organized.
There was a problem hiding this comment.
If performance ever becomes an issue you should consider something like https://turborepo.org/ to speed up execution compared to how npm handles workspaces.
There was a problem hiding this comment.
@gabrocheleau just to give you a bit of a better idea about eslint on a bigger repo. I have a monorepo with turborepo and that repo has 103 packages with over 2000 files, which takes roughly 5s to lint for the whole repo with 4 times the amount of rules.
There was a problem hiding this comment.
Thanks guys for the additional comments! Looks great @faustbrian, I'm on board with the remaining eslint rules! Good to know that there are solutions if we ever run into performance issues.
|
@gabrocheleau think I addressed all of them. |
Looks good, thanks for the quick fixes! I'll wait for additional input from some other team members on the discussion above, and give it another review later this week :) |
|
@holgerd77 Any thoughts on this before we move forward? |
There was a problem hiding this comment.
Approving, looks good to me. Left two minor comments. Also, this should be rebased on latest!
Let's still wait for @holgerd77 's input before merging however.
holgerd77
added
PR state: needs review
type: enhancement
package: monorepo
target: master
warntoerrorbut they require a lot more work because there are hundreds of circular dependencies