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.
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
feat: support additional metadata for rule deprecations #116
feat: support additional metadata for rule deprecations #116
Changes from 1 commit
6d6cc2f
404c101
edb2b76
800b3eb
5c57161
5aaf39f
e18f1f4
4d46d08
e8aa655
9a00eee
16cdc76
2bd065d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
should an empty string be prohibited here, so that “not deprecated” is unambiguously false always?
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.
one of these properties should be required - so like { message } | { url } | { message, url }
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.
[Clarification] Are both
"eslint-plugin-example"
and"example"
allowed?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.
By "shorthand property", I just mean that a string can be provided instead of the more detailed object format.
My intention was for the full plugin name to be provided here
eslint-plugin-foo
or@foo/eslint-plugin
or whatever so there wouldn't be any ambiguity. Simply providing the plugin prefixfoo
doesn't necessarily tell us the full plugin name as there can be various formats...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
meta.replacedBy[].meta
is a bit confusing. It seems like the purpose of thismeta
is really to provide additional info about the replacement. Perhapsnote
orinfo
would be a more appropriate name?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.
info
sounds good to me.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.
same here
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.
[Praise] This is a really good demonstration of the proposed change and its benefits. (and also why eslint-doc-generator is great!)
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.
A typical ESLint deprecation notice includes the version of ESLint that introduced the deprecation, for example "ESLint v8.53.0" for the
semi
rule:I think we want to keep that information, so we would need to store a
deprecatedSince
version key inmeta.replacedBy.info
ormeta.replacedBy.note
, depending on the name we choose, and then use it to generate the notice, if we want to automate that process.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.
[Question] Are there existing examples in the wild of rules that have stayed deprecated for a long time? I'm wondering if there really would be much pain from, say:
meta.deprecated.replacedBy
and markingmeta.replacedBy
as a@deprecated
alias in ESLint v9meta.replacedBy
in ESLint v10The rest of the rule metadata I'd be much more hesitant to change. But this Sourcegraph search for replacedBy in lint/rule files shows only a little under 300 code results, with the occasional script.
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.
Given that i almost never do major bumps, all of the import, react, and jsx-a11y plugins have examples - but many are unlikely to be using replacedBy, since i only learned about it within the last year.
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.
It sounds like @JoshuaKGoldberg has a preference for consolidating the properties, whereas @ljharb is neutral or slightly opposed? I'm open to hearing more opinions and still open to changing the proposal on this based on reviewer feedback.
It is true that a migration from
meta.replacedBy
tometa.deprecated.replacedBy
would be a relatively lightweight and inconsequential migration, compared to more painful ESLint migrations or breaking changes we have performed in the past.I fleshed out this section with some more pros and cons.
Interested to hear more feedback on this decision.
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 mean in general I prefer to avoid breaking changes - in particular because then, how will an eslint plugin support both eslint 9 and 10, given that RuleTester fails with unknown properties?
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.
Another strategy could be to leave it
@deprecated
for two major versions instead of one. Just to give plguins more time to adopt. I'm personally not in favor of this as major versions don't drop very frequently.You could write some quick code around the
RuleTester
usage that adds/delete
s properties as needed. Given how rarely it is for plugins to support quite so long a list of backwards compatibility versions as yours, I imagine the total amount of community work there isn't very high.I think this point also emphasizes how not-very-breaking this is compared to potential other schema changes.
RuleTester
failing with unknown properties only impacts the plugin being tested, right? So even if a few plugins such aseslint-plugin-import
need to do a bit of hackery in tests, I think that'd be justified for the user benefit of a more understandable+unified property.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.
@nzakas @mdjermanovic I'm interested to hear if either of you have a preference between keeping the two existing properties or consolidating
meta.replacedBy
intometa.deprecated.replacedBy
?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'd prefer consolidating
meta.replacedBy
intometa.deprecated.replacedBy
.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.
👍 consolidating.
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.
[Content] From eslint/eslint#18061 (comment):
The "specifying options and changed or missing functionality" point might be relevant. How common is it that a rule deprecation points to a new rule with a particular option?
The only example that comes to mind for me is https://typescript-eslint.io/rules/no-type-alias. It's replaced by two rules:
"interface"
option["TypeAliasDeclaration"]
I personally don't think there's a need for to add functionality to the RFC for options (yet?). There are a lot of edge cases to account for and the
message
string is probably more than sufficient for most of them. But I think it'd be good to mention it as out of scope.Another alternative that could be mentioned is storing a conversion function, the way
tslint-to-eslint-config
does. That in theory could be useful for users who want to get exact conversions... but again, I think is unnecessary work and best to just leave as out of scope.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.
These are great callouts. I added a note about these. I do agree we can probably skip these for now in favor of just using
message
, but open to adding them now or later if someone wants to champion them.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'd prefer to explicitly not encourage discussion of replacements options as part of a rule's meta data. These are subject to change, and keeping that information in sync with a rule in a different repo is a maintenance headache. The source of truth should be in the replacement rule's docs -- the only job of the deprecated rule meta information is to successfully get users from the deprecated rule to the new rule's docs. Whether and which options to use should be found there.