chore: upgrade eslint and fix the warnings #6420
Merged
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
nazarhussain
commented
Feb 12, 2024
.eslintrc.js
Outdated
| @@ -64,6 +64,10 @@ module.exports = { | |||
| }, | |||
| //ignore rules on destructured params | |||
| {selector: "variable", modifiers: ["destructured"], format: null}, | |||
| { | |||
| selector: "import", | |||
| format: ["camelCase", "PascalCase"] | |||
There was a problem hiding this comment.
This will allow to import packages like below without warnings.
import EventEmitter from "node:events";
wemeetagain
reviewed
Feb 12, 2024
nazarhussain
commented
Feb 12, 2024
.eslintrc.js
Outdated
| @@ -159,6 +163,7 @@ module.exports = { | |||
| "prettier/prettier": "error", | |||
| quotes: ["error", "double"], | |||
| semi: "off", | |||
| "import/namespace": "off", | |||
There was a problem hiding this comment.
https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/namespace.md
This rule is taking most of the linting time, 33% of the overall time.
This rule check each package usages at import time, but is not trivial to us as the non-existing methods can also be detected by unsafe-assignment rules.
import/namespace | 10715.327 | 33.2%
There was a problem hiding this comment.
we should probably add a short comment in eslintrc to document it there as well
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6420 +/- ##
=========================================
Coverage 61.72% 61.72%
=========================================
Files 553 553
Lines 57856 57862 +6
Branches 1829 1829
=========================================
+ Hits 35711 35717 +6
Misses 22108 22108
Partials 37 37 |
nflaig
reviewed
Feb 12, 2024
Performance Report✔️ no performance regression detected 🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
nflaig
requested changes
Feb 13, 2024
There was a problem hiding this comment.
Looking at the diff of this PR is makes it pretty clear that import/no-named-as-default and import/no-named-as-default-member are not helpful eslint rules.
The stated rationale in no-named-as-default-member and no-named-as-default.md doesn't sounds convincing to me
Rationale: Accessing a property that has a name that is shared by an exported name from the same module is likely to be a mistake.
Named import syntax looks very similar to destructuring assignment. It's easy to make the (incorrect) assumption that named exports are also accessible as properties of the default export.
I don't see how this prevents actual issues especially since we use typescript which won't allow us to access properties that do not exist. Looking at the diff of the PR, those rules look like a net negative to me.
Maybe someone can make a strong argument why we should keep those, else I would suggest to disable them
The rule and the rule |
|
That's what the examples show in the .md files of the rules as well but I don't see how that is useful in a typed system why is this import {stringify as queryStringStringify} from "qs";
queryStringStringify(...)better than this import qs from "qs";
qs.stringify(...)if the qs package has a default export then second example looks perfectly fine to me, in fact, the syntax is more readable / concise. Ideally, the function call should be clear without looking at imports. And having Is there one example in our code where those rules are helpful? |
Because you import more than what you need, which is a bad practice. The whole package may come with some side effects. So why destructured import and named exports was first introduced in ES and so why it's considered |
Unless I completely misunderstand how ES modules are loaded there is no difference in destructuring or not, this seems to be confirmed here. And to take advantage of tree shaking I think it is only possible if the package architecture allows for it.
The stated rationale of those rules has nothing to do what you just mentioned, maybe we need a rule then that forces you to always destructure imports but sounds like a big pain to me as native nodejs libs are designed (for better semantics) to be used via default export |
nflaig
approved these changes
Feb 13, 2024
Destructruing allows to remove to dead code and production builds should strip all such cases. CommonJS didn't allowed that which is referred in your link, so why ES is considered better JS modules approach. And any module may have side effect or not, or our building process handle dead code or not, one can't think of all such cases while writing the code. While writing you just follow the best practices and leave rest extra tooling and future improvements in that tooling. Using destructuring imports for named exports is commonly known best practice, so why added into linting recommended config. And when we discuss import specific rules, we don't include CommonJS in that discussion. |
|
@nazarhussain interesting article about tree shaking The conclusion
And for commonjs packages you will have to import from like this So again, I don't see how those rules help us here 🤷 |
Tree shaking and detecting dead code behavior is slightly different among builders e.g. Webpack, ES-Build, Rollup, TSC. While writing code you don't worry about which tool is used and what future improvements in those tools are expected to appear. Any future release of Webpack may improve some edge case which it does not before and you would not like to rewrite your code when it does. So if you follow a practice recommended by community, tools like Webpack will keep improving around those practices. |
|
🎉 This PR is included in v1.16.0 🎉 |
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.
Motivation
Make the core more reliable.
Description
Fixing all the warnings in the code increase more reliability and trust on it.
Steps to test or reproduce