New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Warn on deprecated rules #7443
Comments
Thanks so much for the detailed summary! I agree we should definitely do this. However, since this is a core change, it requires approval from our Technical Steering Committee. I'll work on adding this to the agenda, and we can hopefully discuss in the next TSC meeting (which I think is this Thursday). |
A couple points of reference:
|
TSC Summary: We currently support warnings for rules that have been outright removed (and only core rules). Now that we have meta.deprecated, users may wish to see that a core or plugin rule has been deprecated. TSC Question: Should we augment ESLint core to generate a warning message when rules are deprecated? If so, should we support more metadata fields to allow rule maintainers to indicate the upgrade path and/or removal timeline? |
The rules that currently have |
I'll champion this. (Would have done so earlier but thought that was redundant if TSC evaluated and possibly approved) |
There are concerns this could be a breaking change since it could impact CI environments and editors. It will be discussed again at the next meeting. |
I intend to either work with @taion to write a POC PR, or to write it myself, hopefully before the next TSC meeting two weeks from today. @taion Please let me know what your availability might be for working on a POC pull request (and if the answer is "not available", that's absolutely fine). Thanks! |
I could take a stab at it. I'm not familiar with what the safe way to do this would be. Personally, I haven't experienced any problems with the way ESLint-plugin-React warns on deprecated rules, but using |
I'm thinking 2 approaches here.
I think A is much noisy. If people run an
|
I like B. The problem with A in my view is that adding a warning seems like a non-backward-compatible change, and by convention deprecations are usually handled as semver-minor. |
If we go with option B, we have to make sure it's backwards compatible. There are custom formatters out there, and we can't break them with this change. |
I'm not sure the concerns about a "breaking change" are warranted for option A. This only affects users who are using rules that have been marked as deprecated, which is only 3-4 out of 200+ core rules plus the small fraction of plugins who are using that capability. The proposal in option A is to use warnings, which will not break CI builds. And we know formatters know how to handle warnings, so there's no breakage there. And on top of all of that, the rules are already deprecated, so we're doing the user a favor and telling them they need to find a replacement. So as far as I'm concerned, this is a semver-minor new feature. That said, I appreciate that the editor experience needs to be considered. But it can be considered without deciding this whole thing is a breaking change. If this affected all rules, that would be one thing. But semver-major is only required when the "public API" is changed in a backward incompatible way, and Option A is not changing the public API, only deprecated APIs. |
To clarify, my concern there is from the perspective of plugins. I'd like to e.g. be able to bump a minor version on eslint-plugin-babel to deprecate rules with changes that have been upstreamed. If it's generally agreed that adding ESLint warnings is non-breaking, then I'd be okay with that. |
In the 2016-11-10 TSC meeting, the team decided in favor of including this feature. |
Speaking for myself and not the team now, I'm a fan of @mysticatea's option B. @ilyavolodin are you concerned that adding the additional object keys could break formatters? |
That link looks odd – should it have been https://gitter.im/eslint/tsc-meetings/archives/2016/11/10? |
@taion indeed, thanks for catching that! Fixed. |
I am not sure we can assume a warning won't break builds. I can easily imagine people having warnings as ok locally during development (e.g. console.log) but failing on CI. |
@btmills No, not really concerned, just saying we need to make sure that third-party formatters will be able to ignore new properties without breaking them. |
@ilyavolodin @vitorbal @not-an-aardvark Where do you believe we are on this? Have we agreed on an implementation yet? |
I think we're mostly leaning towards option B, but I'm still curious what
other people think about the question I raised on my previous comment.
…On Tue, Nov 29, 2016 at 7:10 PM Kevin Partington ***@***.***> wrote:
@ilyavolodin <https://github.com/ilyavolodin> @vitorbal
<https://github.com/vitorbal> @not-an-aardvark
<https://github.com/not-an-aardvark> Where do you believe we are on this?
Have we agreed on an implementation yet?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7443 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAmNdmI7JfKlety8up6ZMlEVR5K_8vEIks5rDGqGgaJpZM4KfDvf>
.
|
Okay, I'll support option B as well. @taion Are you by any chance interested in working on this? Thanks everyone for your patience-- unfortunately we get a lot of issues and sometimes these things slip through the cracks. |
Unfortunately, I don't think I'm going to have time in the near future – I have a number of work deadlines coming up in the next few weeks. |
I'd prefer option B as well. It's less obtrusive and won't detract from actual code issues during development, should one choose to stick with the deprecated rule for the moment. That being said, if nobody with more insight will tackle this in the meantime, I'm hoping to have some time for this in March. I would probably need some pointers on where to look, though 🤔 |
Jumping into this a bit late, but is this still a breaking change? I see it's in the v4.0.0 GitHub Project but doesn't have a breaking label. Want to make sure we're not holding up v4 for an issue that may not be a breaking change. |
I don't think this would be a breaking change if the only thing we do is
add the list of deprecated rules to the result of the Node.js API.
We can open a separate issue/PR for modifying some of our formatters to
print the list of deprecated rules (this second step would be a breaking
change).
…On Sat, Mar 11, 2017 at 9:41 PM Kai Cataldo ***@***.***> wrote:
Jumping into this a bit late, but is this still a breaking change? I see
it's in the v4.0.0 GitHub Project but doesn't have a breaking label. Want
to make sure we're not holding up v4 for an issue that may not be a
breaking change.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7443 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAmNdslRPzVatWNvvZbVdkM2yuLF2Mz0ks5rkwcOgaJpZM4KfDvf>
.
|
It looks like the resolution is to expose deprecated rules in the Node API, but not change the CLI output for now, so this isn't a breaking change. I'll remove it from the 4.0 project. |
@eslint/eslint-team Is this something we could revisit, using a framework similar to what we did for #10230? |
As of v3.8.1, ESLint has a number of deprecated rules:
These rules are marked as
deprecated
inmeta
.However, there's no easy way for users to know that these rules are deprecated without explicitly following ESLint development.
There is a capacity for warning about the use of rules that were previously removed and replaced by other rules, but this only applies to first-party rules, and only to ones that have actually been removed: https://github.com/eslint/eslint/blob/v3.8.1/lib/eslint.js#L818-L827
I believe it would be helpful to check
ruleCreator.meta.deprecated
at that location, then emit a warning if that rule is deprecated.This would be helpful for third-party plugins as well; the impetus for this is my wanting to deprecate some rules in eslint-plugin-babel that now have their functionality covered by rules in ESLint proper.
The text was updated successfully, but these errors were encountered: