This repository has been archived by the owner on Oct 16, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 39
feat: recommended config #69
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
import recommended from './recommended'; | ||
|
||
export { recommended }; |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
const recommended = { | ||
plugins: ['deprecation'], | ||
rules: { | ||
'deprecation/deprecation': 'error', | ||
}, | ||
}; | ||
|
||
export default recommended; |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
import * as rules from './rules'; | ||
import * as configs from './configs'; | ||
|
||
export { rules }; | ||
export { configs, rules }; |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to use warning as a recommended configuration and let users to opt in into a more strict mode manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the industry best-practice is to use error.
For example, see the ESLint recommended rules, which contain no warnings.
Also, see the TypeScript-ESLint recommended rules, which contain no warnings.
Also, see the Unicorn recommended rules, which contain no warnings.
And so on.
Personally, in my projects, I use eslint-plugin-only-warn, which converts all errors to warnings, making the distinction superfluous. The reason is because I like red squigglys to be from the TypeScript compiler, and yellow squigglys to be from ESLint, which provides a nice delineation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I agree that majority of eslint rules should be errors but deprecation for me seems more like a thing to keep an eye on and not necessarily a bad thing which is fine to have in your code and not fail your CI.
And simply forcing ppl to ignore their deprecations because the rule is too strict is eventually going to lead to a more deprecated code being accumulated and lost track of as eslint will no longer report those.
I know it's probably opinionated but I guess that's the whole point of "recommended" config anyway =)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the point of my previous explanation was that I think that all ESLint rules should be errors, not just "most". If users want to explicitly not follow ESLint best practice and use
warn
, then they should do that by not using therecommended
config and manually configure the rule themselves.I respectfully disagree! =p I think failing in CI is a fantastic time to know about deprecations. What is the alternative? For the ESLint warning to only show up in VSCode if one of the programmers on your project happens to open the file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More thoughts:
I think a common workflow for TypeScript projects is to update dependencies in
package.json
, make a commit, push to GitHub, and then see if CI passes. (TypeScript compiler might fail on changed/deleted library functions, and if TypeScript compiler passes all the code is probably okay.)This is the exact moment in time where I want to first find out that my library usage is deprecated. By not having CI fail with a deprecation, the deprecation is going to be hidden until N days in the future, when I am busy dealing with other things, and reverting deps in the project might not be possible anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I see your points, and I do agree that errors are better for discovery, because literally nobody ever reads warnings if everything passes.
But deprecations are a bit different can of worms than a simple code style rule which you may not be able to resolve that easily. And because deprecations are still a valid code that will work until some time in the future, it is totally valid to keep it for now until you have plans to prepare for a future version of whatever you are using.
This is why I see it as a warning mostly - it's okay to use that piece of code but you have to know that at some point in the future you will have to refactor. This is the definition of the warning to me. And I would not want to add those "eslint-ignore" comments just because the rule is going to crash CI.
If you are still convinced that errors are better for deprecations - I'm fine to merge it as is, but personally I would not use "recommended" version in my projects 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my mind, the primary purpose of
eslint-plugin-deprecation
is discovery. I want to discover when library functions become deprecated, so that I can change my code. All the other points don't matter as much as discovery.I would prefer to merge as is, but we can leave this issue open for a while to see what other developers and users of
eslint-plugin-deprecation
think. I predict that most other people in the ecosystem will not usewarn
for this rule (or for any other rule for that matter), but I'm open to other perspectives.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use the rule with
warn
, but execute ESLint with--max-warnings 0
.Note that using
max-warnings 0
isn't only about deprecations. We also think that warnings can easily be overlooked, especially when trusting the CI. Thus, we fail on warnings.I still think that deprecations should be warnings, because there may not be an immediate action required and solving a deprecation could be postponed by increasing the
--max-warnings
without ignoring the finding altogether.