feat: recommended config #69
Conversation
| const recommended = { | ||
| plugins: ['deprecation'], | ||
| rules: { | ||
| 'deprecation/deprecation': 'error', |
There was a problem hiding this comment.
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.
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.
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.
In general I agree that majority of eslint rules should be errors
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 the recommended config and manually configure the rule themselves.
not necessarily a bad thing which is fine to have in your code and not fail your CI.
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?
- What if its a file that hasn't been touched in years? What are the chances that someone is going to randomly open it in their code editor?
- Or, alternatively, what are the chances that someone is going to randomly notice the warning in CI by reading CI logs for a non-failed CI run?
- It seems much more likely that they will first discover the warning when reading the CI logs for an actual failed CI run, which causes unnecessary noise and actively wastes someone's time when trying to get to the root cause of the issue causing the actual failure.
- Or, alternatively, what are the chances that someone is going to manually run lint on their local computer to discover the warning when linting is already automatically set up in CI?
There was a problem hiding this comment.
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.
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.
I do agree that errors are better for discovery
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 use warn 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.
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.
|
🎉 This PR is included in version 1.5.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
+1 for warning instead of error |
Closes #68