-
-
Notifications
You must be signed in to change notification settings - Fork 67
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: additional rule metadata for deprecations #124
base: main
Are you sure you want to change the base?
Conversation
type ReplacementKind = | ||
'moved' | // The rule has moved to another plugin if plugin is set, otherwise the rule is renamed in the same plugin | ||
'merged' | // The rule merged with another rule | ||
'option' // The current rule behavior is available as an option in the replacement rule |
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 ReplacedByInfo.kind
isn't necessary as this info and more context can be provided as part of the ReplacedByInfo.info
(or ReplacedByInfo.info.message) text.
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.
The idea behind this property was:
- for documentation generators: able to provide a default "message" without having to specify the same message for multiple rules (e.g. the rule was renamed from "foo" to "bar")
- a possible config migration tool: suggest changes without relying on a specific format in info.message (e.g. rename the rule in the config)
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'm also not sure of the value here. A single word just doesn't given enough context. I also fear this would become a maintenance issue as we'll end finding other reasons rules were deprecated and need to update this list with more unclear terms.
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.
Looks like we're in favor of removing this property. @DMartens can you update the RFC?
5. Which "extension points" (rules, processors, configurations, parsers, formatters) shold be supported? | ||
6. Should the `rule` key be dependent on the "extension point" (e.g. `processor` for processors) or renamed (e.g. ``) so that it is the same property name for all? |
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 the scope of this RFC is just rules deprecations. But if we are already anticipating that other extension points (processors, configurations, parsers, formatters) will have deprecation info, with the same shape, then a more generic name like replacement
might have sense, so that it's the same property name for all.
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 do not know of examples for other extension points being deprecated but especially with the introduction of language plugins, I can see some processors being deprecated (e.g. eslint-plugin-markdown).
}; | ||
|
||
/* At least one property is required */ | ||
type DeprecateInfo = { |
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.
type DeprecateInfo = { | |
type DeprecatedInfo = { |
kind?: ReplacementKind // Defaults to "moved" if missing | ||
} | ||
|
||
type Message = { |
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'm not sure this type is necessary. I'd rather just move message
and url
to DeprecatedInfo
and ReplacedByInfo
. Remove one level of objects.
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 also think it would be better to simplify this by moving these properties directly into DeprecatedInfo
and ReplacedByInfo
.
type ReplacementKind = | ||
'moved' | // The rule has moved to another plugin if plugin is set, otherwise the rule is renamed in the same plugin | ||
'merged' | // The rule merged with another rule | ||
'option' // The current rule behavior is available as an option in the replacement rule |
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'm also not sure of the value here. A single word just doesn't given enough context. I also fear this would become a maintenance issue as we'll end finding other reasons rules were deprecated and need to update this list with more unclear terms.
rule: 'https://eslint.style/rules/js/semi', | ||
}, |
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.
Syntax error
message: 'Stylistic rules are being moved out of ESLint core.', | ||
url: 'https://eslint.org/blog/2023/10/deprecating-formatting-rules/', |
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.
message
and url
aren't listed as members above (although, I do like this better. See my earlier comments.)
--> | ||
|
||
1. Is there additional deprecation information we'd like to represent? Note that additional information can always be added later, but it's good to consider any possible needs now. | ||
2. Should `meta.deprecated.plugin.id` accommodate different package registries (e.g. [jsr](https://jsr.io/) with `jsr:eslint-plugin-example`) |
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.
2. Should `meta.deprecated.plugin.id` accommodate different package registries (e.g. [jsr](https://jsr.io/) with `jsr:eslint-plugin-example`) | |
2. Should `meta.deprecated.plugin.name` accommodate different package registries (e.g. [jsr](https://jsr.io/) with `jsr:eslint-plugin-example`) |
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 don't think we should worry about package registries. People should include a URL to the package wherever its published if this is a concern.
deprecatedSince?: Version // Helps users gauge when to migrate and useful for documentation | ||
availableUntil?: Version | null // The estimated version when the rule is removed (probably the next major version). null means the rule is "frozen" (will be available but will not be changed) |
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.
Will the Version
format be the same as the value as in package.json, or will it include a v
in the beginning (e.g. "v9.11.1"
)?
Also, let's suppose that we decide to deprecate a rule in ESLint v10. What value should be set for deprecatedSince
? Should it be "v10.0.0-alpha.0"
,"v10.0.0"
, "v10"
, etc.?
I believe in practice every plugin will end up using these fields slightly differently, but it would be useful to include usage examples for ESLint specifically.
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.
Also, let's suppose that we decide to deprecate a rule in ESLint v10. What value should be set for
deprecatedSince
? Should it be"v10.0.0-alpha.0"
,"v10.0.0"
,"v10"
, etc.?
10.0.0
. Prereleases don't count and we should use the full semver version.
Co-authored-by: Milos Djermanovic <[email protected]>
name?: string // Name of the rule / configuration / ... | ||
url?: string // URL to more information about this deprecation in general. |
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.
name?: string // Name of the rule / configuration / ... | |
url?: string // URL to more information about this deprecation in general. | |
name?: string // Name of the plugin / rule / ... | |
url?: string // URL to documentation for the plugin / rule / ... |
External changes: | ||
|
||
- Update the [types](https://github.com/DefinitelyTyped/DefinitelyTyped/blob/b77d83e019025017b06953907cb77f35e4231714/types/eslint/index.d.ts#L734) in @types/eslint |
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.
This would be a change inside ESLint, because the eslint
package now includes types:
'merged' | // The rule merged with another rule | ||
'option' // The current rule behavior is available as an option in the replacement rule | ||
|
||
/* Version string of the package containing the rule */ |
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.
/* Version string of the package containing the rule */ | |
/* Version string. It should be the full semver version without the "v" prefix, e.g. "8.53.0" */ |
Per the discussion on #124 (comment)
- Ensure these properties are allowed on configurations, parsers and processors | ||
- Add any additional information to these properties in core rules as desired (such as in <https://github.com/eslint/eslint/issues/18053>) | ||
- Update ESLint's website generator to take into account the additional information for rule doc deprecation notices | ||
- Update [LintResult.usedDeprecatedRules](https://github.com/eslint/eslint/blob/0f5df509a4bc00cff2c62b90fab184bdf0231322/lib/eslint/eslint.js#L197-L211) |
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.
Can you add more details about this change? Shall we keep the existing replacedBy: string[]
property and/or add a new one with the DeprecatedInfo
object?
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 removing or changing the format of DeprecatedRuleInfo.replacedBy
could break existing tools like custom formatters. Maybe it would be better to expose the RuleMeta.deprecated
object as a new property of DeprecatedRuleInfo
?
In terms of actual changes inside ESLint needed for this: | ||
|
||
- Mention the new schema in the [custom rule documentation](https://eslint.org/docs/latest/extend/custom-rules#rule-structure) | ||
- Ensure these properties are allowed on configurations, parsers and processors |
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.
If this RFC only aims to add support for additional metadata for rule deprecations, then I think this change is out of its scope.
- Ensure these properties are allowed on configurations, parsers and processors | ||
- Add any additional information to these properties in core rules as desired (such as in <https://github.com/eslint/eslint/issues/18053>) | ||
- Update ESLint's website generator to take into account the additional information for rule doc deprecation notices | ||
- Update [LintResult.usedDeprecatedRules](https://github.com/eslint/eslint/blob/0f5df509a4bc00cff2c62b90fab184bdf0231322/lib/eslint/eslint.js#L197-L211) |
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 removing or changing the format of DeprecatedRuleInfo.replacedBy
could break existing tools like custom formatters. Maybe it would be better to expose the RuleMeta.deprecated
object as a new property of DeprecatedRuleInfo
?
name: '@stylistic/js', | ||
url: 'https://eslint.style/', | ||
}, | ||
rule: 'https://eslint.style/rules/js/semi', |
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.
name: '@stylistic/js', | |
url: 'https://eslint.style/', | |
}, | |
rule: 'https://eslint.style/rules/js/semi', | |
name: '@stylistic/js', | |
url: 'https://eslint.style/', | |
}, | |
rule: 'https://eslint.style/rules/js/semi', |
To fix the indentation.
you can remove this section. | ||
--> | ||
|
||
1. Is there additional deprecation information we'd like to represent? Note that additional information can always be added later, but it's good to consider any possible needs now. |
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.
Is there additional deprecation information we'd like to represent?
Not for ESLint directly as far as I know, but third-party plugins may want to include additional deprecation information for their specific purposes. One use case that's been already mentioned in https://github.com/eslint/rfcs/pull/124/files#r1764772326 is the ability to mark a rule that was renamed. Another use case is to include the commit hash or PR link in which a rule was deprecated, like shown in the typescript-eslint
docs (an example).
My suggestion is that we only focus on information consumed by ESLint in this RFC (hence no ReplacementKind
needed). But I'd also suggest that we don't try to validate or forbid unrecognized properties in a meta.deprecated
object.
Summary
This is a continuation of #116 by @bmish to specify additional properties for rule deprecations.
The main changes are:
meta.deprecated
and deprecatemeta.replacedBy
kind
,deprecatedSince
,availableUntil
Related Issues