feat: support additional metadata for rule deprecations #116

bmish · 2024-02-21T00:56:05Z

Summary

This RFC suggests a format for storing additional information in rule metadata about rule deprecations and replacement rules, allowing automated documentation and website tooling to generate more informative deprecation notices.

Related Issues

Change Request: Support additional rule metadata for deprecations eslint#18061

ljharb

This looks great!

ljharb · 2024-02-21T02:01:45Z

designs/2024-deprecated-rule-metadata/README.md

+type RuleMeta = {
+  deprecated:
+    | boolean // Existing boolean option, backwards compatible.
+    | string // Shorthand property for general deprecation message, such as why the deprecation occurred. Any truthy value implies deprecated.


should an empty string be prohibited here, so that “not deprecated” is unambiguously false always?

ljharb · 2024-02-21T02:02:15Z

designs/2024-deprecated-rule-metadata/README.md

+    | string // Shorthand property for general deprecation message, such as why the deprecation occurred. Any truthy value implies deprecated.
+    | {
+        message?: string; // General deprecation message, such as why the deprecation occurred.
+        url?: string; // URL to more information about this deprecation in general.


one of these properties should be required - so like { message } | { url } | { message, url }

ljharb · 2024-02-21T02:02:44Z

designs/2024-deprecated-rule-metadata/README.md

+            };
+        meta?: {
+          message?: string; // Message about this specific replacement, such as how to use/configure the replacement rule to achieve the same results as the rule being replaced.
+          url?: string; // URL to more information about this specific deprecation/replacement.


designs/2024-deprecated-rule-metadata/README.md

JoshuaKGoldberg

LGTM, with the strong caveat that I've never reviewed an RFC as a member of the committer team before and thus should not be taken very seriously. 😄

Very excited about this one though! I think it'll be great for docs generation & helping folks migrate to new rules. 🚀

JoshuaKGoldberg · 2024-02-21T12:26:24Z

designs/2024-deprecated-rule-metadata/README.md

+         * Omit if the replacement is in the same plugin.
+         */
+        plugin?:
+          | string // Shorthand property for the plugin name i.e. "eslint-plugin-example" that contains the replacement rule.


[Clarification] Are both "eslint-plugin-example" and "example" allowed?

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 prefix foo doesn't necessarily tell us the full plugin name as there can be various formats...

JoshuaKGoldberg · 2024-02-21T12:27:17Z

designs/2024-deprecated-rule-metadata/README.md

+
+> semi (deprecated) \
+> Replaced by [semi](https://eslint.style/rules/js/semi) from [@stylistic/js](https://eslint.style/). \
+> Stylistic rules are being moved out of ESLint core. [Read more](https://eslint.org/blog/2023/10/deprecating-formatting-rules/).


[Praise] This is a really good demonstration of the proposed change and its benefits. (and also why eslint-doc-generator is great!)

A typical ESLint deprecation notice includes the version of ESLint that introduced the deprecation, for example "ESLint v8.53.0" for the semi rule:

This rule was deprecated in ESLint v8.53.0. Please use the corresponding rule in @stylistic/eslint-plugin-js.

I think we want to keep that information, so we would need to store a deprecatedSince version key in meta.replacedBy.info or meta.replacedBy.note, depending on the name we choose, and then use it to generate the notice, if we want to automate that process.

JoshuaKGoldberg · 2024-02-21T12:36:24Z

designs/2024-deprecated-rule-metadata/README.md

+2. Consolidate `meta.replacedBy` into `meta.deprecated.replacedBy`. This alternative was strongly considered. If we were to design things from scratch, we might choose this approach. However, there's not much tangible benefit to it besides organizational tidiness, and the downsides include:
+   - It creates yet another migration burden for the plugin ecosystem (potentially hundreds of plugins) to migrate from the old property to the new property
+   - There's overhead for us to go through the process of deprecating the old property and encouraging or helping plugins to move to the new one
+   - Tooling would need to support both properties, perhaps indefinitely due to the long tail of rules that may never be updated


[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:

Adding meta.deprecated.replacedBy and marking meta.replacedBy as a @deprecated alias in ESLint v9

Removing meta.replacedBy in ESLint v10

The 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.

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.

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 to meta.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.

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?

support both eslint 9 and 10

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.

given that RuleTester fails with unknown properties

You could write some quick code around the RuleTester usage that adds/deletes 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 as eslint-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.

@nzakas @mdjermanovic I'm interested to hear if either of you have a preference between keeping the two existing properties or consolidating meta.replacedBy into meta.deprecated.replacedBy?

I'd prefer consolidating meta.replacedBy into meta.deprecated.replacedBy.

👍 consolidating.

JoshuaKGoldberg · 2024-02-21T12:41:36Z

designs/2024-deprecated-rule-metadata/README.md

+    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.


[Content] From eslint/eslint#18061 (comment):

The only other point that comes to mind for me is being able to specify options in the new rule. https://github.com/typescript-eslint/tslint-to-eslint-config -> https://github.com/typescript-eslint/tslint-to-eslint-config/blob/470d44de20beb7c7366de993edb8898d0766b8aa/docs/Architecture/Linters.md might be a good reference. Deprecated rules could map to multiple new ones, including specifying options and changed or missing functionality.

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:

Behavior change, but recommended: https://typescript-eslint.io/rules/consistent-type-definitions with the default "interface" option

Preserving the original behavior: https://eslint.org/docs/latest/rules/no-restricted-syntax, with an option like ["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.

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.

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.

Co-authored-by: Josh Goldberg ✨ <[email protected]>

bmish · 2024-02-21T19:45:39Z

designs/2024-deprecated-rule-metadata/README.md

+Pros:
+
+- Organizational tidiness
+- Avoids the inconsistent situation where `meta.deprecated` is `false` or omitted but `meta.replacedBy` has a value, resulting in ambiguity about whether the rule is deprecated or not


This is an open design question. If we stick with two separate properties, what does it mean for meta.deprecated to be false or omitted but meta.replacedBy to contain a value? It seems implied that a rule that has replacements would be deprecated, but it might not be explicitly marked as so which is concerning.

Perhaps this is an opportunity for a RuleTester check?

Thinking from the perspective of types: this is tough to represent precisely in TypeScript. Either...

... the types are straightforward ({ deprecated?: boolean; replacedBy?: ReplaceBy }) and allow not-good situations ({ replacedBy: [...] })

... the types are something more complex such as a union of objects, and are more difficult to understand

Complex types are generally a symptom of complex code ideas. Which I think is true here. It's not intuitive to answer these questions.

Not saying that TypeScript specifically is a reason to do anything 😄 - just that its type system is a good way to sniff out conceptual complexity.

nzakas

Overall this LGTM. Just left a few notes on various things, but overall I think this is a well-thought-out proposal that could be implemented without disruption.

nzakas · 2024-02-27T17:48:37Z

designs/2024-deprecated-rule-metadata/README.md

+              name?: string; // Replacement rule name (without plugin prefix).
+              url?: string; // URL to rule documentation.
+            };
+        meta?: {


I think meta.replacedBy[].meta is a bit confusing. It seems like the purpose of this meta is really to provide additional info about the replacement. Perhaps note or info would be a more appropriate name?

info sounds good to me.

nzakas · 2024-02-27T17:50:26Z

designs/2024-deprecated-rule-metadata/README.md

+Pros:
+
+- Organizational tidiness
+- Avoids the inconsistent situation where `meta.deprecated` is `false` or omitted but `meta.replacedBy` has a value, resulting in ambiguity about whether the rule is deprecated or not


Perhaps this is an opportunity for a RuleTester check?

nzakas · 2024-02-27T17:52:25Z

designs/2024-deprecated-rule-metadata/README.md

+    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.


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.

mdjermanovic · 2024-04-29T13:43:19Z

designs/2024-deprecated-rule-metadata/README.md

+## Backwards Compatibility Analysis
+
+<!--
+    How does this change affect existing ESLint users? Will any behavior
+    change for them? If so, how are you going to minimize the disruption
+    to existing users?
+-->
+
+Existing rules will continue to be backwards-compatible with the new format.
+
+Changing the format of these properties mainly affects third-party documentation tooling and websites that use this information, and not ESLint users nor ESLint plugins directly.
+
+For the most part, the new `meta.deprecated` format should be backwards-compatible, as code is often written to check simply for a truthy value inside of `meta.deprecated`, e.g. `if (rule.meta.deprecated) { /* ... */ }`, which will continue to work as expected. If code checks specifically for the boolean `true` value in `meta.deprecated`, or retrieves rule names from `meta.replacedBy`, then it would need to be updated.
+
+Overall, a limited number of third-party tools that might be affected, and these should be trivial to fix when impacts are discovered.
+
+We do not need to consider this to be a breaking change in terms of [ESLint's semantic versioning policy](https://github.com/eslint/eslint#semantic-versioning-policy).


Would we keep the same type of the usedDeprecatedRules property on LintResult objects that are returned from ESLint APIs and passed to formatters?

Currently, it's:

usedDeprecatedRules ({ ruleId: string; replacedBy: string[] }[])

If we keep the same type, then we'd need to update the code that generates this property. If we change the type so that replacedBy is returned as-is (array of strings or an object) from rule meta, then it could be considered as a breaking change.

aladdin-add · 2024-05-10T09:49:00Z

@bmish there are a few more comments here. :)

mdjermanovic · 2024-05-21T10:53:48Z

@bmish just to check, are you still working on this?

nzakas · 2024-06-03T14:53:21Z

@bmish Just checking in. Are you still working on this?

nzakas · 2024-06-24T14:57:12Z

@bmish there are some outstanding comments to address. Are you still interested in finishing this RFC?

bmish · 2024-07-03T23:37:49Z

I'd like to finish it up but it might be a while until I find some time.

nzakas · 2024-07-04T15:50:51Z

@bmish can you give us a rough timeframe so we can know when to follow up?

nzakas · 2024-08-07T15:08:13Z

@bmish it's been a month since the last ping. Do you have a timeframe for when you plan on finishing this up?

nzakas · 2024-08-08T20:17:42Z

Hi everyone - we're looking for someone who's interested in this RFC to pick it up and push it over the finish line. If you're interested, please leave a comment here.

DMartens · 2024-08-09T15:15:18Z

I'm interested in finishing this.
I'm in favor of only using the deprecated key as there is a strict relationship between deprecated and replacedBy and
checking whether the legacy form is used is simple: if (rule.meta.deprecated || "replacedBy" in rule.meta).
With that and the notes from others it would like this:

type RuleMeta = {
	deprecated?: {
		replacedBy?: ...
		plugin?: string | { name?: string, url?: string } // Question: Should it be required that the name is its id on npm? or allow other registries such as jsr. Also worth documenting is if it should be set if it is from the same plugin
		rule?: string | { name?: string, url?: string }
		info?: string | { message?: string, url?: string } // Originally "meta"

		// Additions
		deprecatedSince?: string // Version string since when the rule is deprecated
		availableUntil?: null | string // Version or date when the rule will be removed or null if the rule is frozen
	}
}

Another thing worth mentioning is we could get rid of requiring one property (either url or name) in the object forms if the shorthand string should be interpreted smartly: if it starts with a protocol (e.g. https:// it is shorthand for the url and otherwise for message or name. Then the object forms would require both properties to be set.

nzakas · 2024-08-09T15:36:26Z

@DMartens I'd say put all of those thoughts into the RFC and we can go from there.

fasttime · 2024-08-12T07:27:56Z

@DMartens Thanks for picking up the work on this RFC. Feel free to open a new pull request as you probably don't have write access to the source repo here.

nzakas · 2024-09-11T14:40:23Z

@DMartens are you still interested in picking this up?

DMartens · 2024-09-12T21:51:30Z

Sorry, I wanted to proofread it the next day and forgot. Anyways the proposal is continued in #124.

nzakas · 2024-09-13T14:23:01Z

No worries. We'll close this and continue the discussion on #124.

eslint-github-bot bot added the feature label Feb 21, 2024

bmish force-pushed the deprecated-rule-metadata branch from bb7efa2 to 28bb87f Compare February 21, 2024 00:56

This was referenced Feb 21, 2024

Change Request: Support additional rule metadata for deprecations eslint/eslint#18061

Open

Automate docs with eslint-doc-generator jsx-eslint/eslint-plugin-react#3469

Merged

feat: support additional metadata for rule deprecations

6d6cc2f

bmish force-pushed the deprecated-rule-metadata branch from 28bb87f to 6d6cc2f Compare February 21, 2024 00:59

ljharb approved these changes Feb 21, 2024

View reviewed changes

tweak

404c101

JoshuaKGoldberg reviewed Feb 21, 2024

View reviewed changes

designs/2024-deprecated-rule-metadata/README.md Outdated Show resolved Hide resolved

JoshuaKGoldberg reviewed Feb 21, 2024

View reviewed changes

designs/2024-deprecated-rule-metadata/README.md Outdated Show resolved Hide resolved

JoshuaKGoldberg approved these changes Feb 21, 2024

View reviewed changes

bmish and others added 6 commits February 21, 2024 10:21

Update designs/2024-deprecated-rule-metadata/README.md

edb2b76

Co-authored-by: Josh Goldberg ✨ <[email protected]>

Update designs/2024-deprecated-rule-metadata/README.md

800b3eb

Co-authored-by: Josh Goldberg ✨ <[email protected]>

tweaks

5c57161

tweak

5aaf39f

tweak

e18f1f4

tweak

4d46d08

nzakas added the Initial Commenting This RFC is in the initial feedback stage label Feb 21, 2024

bmish added 4 commits February 21, 2024 14:08

out of scope options/conversion function

e8aa655

tweak

9a00eee

alternatives flesh out

16cdc76

flesh out

2bd065d

bmish commented Feb 21, 2024

View reviewed changes

nzakas approved these changes Feb 27, 2024

View reviewed changes

mdjermanovic reviewed Apr 29, 2024

View reviewed changes

DMartens mentioned this pull request Sep 12, 2024

feat: additional rule metadata for deprecations #124

Merged

nzakas closed this Sep 13, 2024

feat: support additional metadata for rule deprecations #116

feat: support additional metadata for rule deprecations #116

Conversation

bmish commented Feb 21, 2024

Summary

Related Issues

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

nzakas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aladdin-add commented May 10, 2024

mdjermanovic commented May 21, 2024

nzakas commented Jun 3, 2024

nzakas commented Jun 24, 2024

bmish commented Jul 3, 2024

nzakas commented Jul 4, 2024

nzakas commented Aug 7, 2024

nzakas commented Aug 8, 2024

DMartens commented Aug 9, 2024

nzakas commented Aug 9, 2024

fasttime commented Aug 12, 2024

nzakas commented Sep 11, 2024

DMartens commented Sep 12, 2024

nzakas commented Sep 13, 2024

JoshuaKGoldberg left a comment •

edited

Loading

ljharb Feb 21, 2024 •

edited

Loading

JoshuaKGoldberg Feb 27, 2024 •

edited

Loading