-
Notifications
You must be signed in to change notification settings - Fork 13
Support new DeprecatedInfo format for rule meta.deprecated
#730
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
base: main
Are you sure you want to change the base?
Support new DeprecatedInfo format for rule meta.deprecated
#730
Conversation
lib/rule-doc-notices.ts
Outdated
| fixable: boolean; | ||
| hasSuggestions: boolean; | ||
| urlConfigs?: string; | ||
| deprecatedInfo: boolean | DeprecatedInfo | undefined; |
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.
Why is boolean included 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.
Typescript would complain about it when it's passed to ruleNoticeStrOrFn (ts2322) because meta?.deprecated still includes the boolean type
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.
Let's call this deprecated to match the rule property name.
lib/rule-doc-notices.ts
Outdated
| ) | ||
| .filter((rule): rule is string => typeof rule === 'string'); | ||
|
|
||
| return `${EMOJI_DEPRECATED} This rule is deprecated${ |
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.
| return `${EMOJI_DEPRECATED} This rule is deprecated${ | |
| return `${EMOJI_DEPRECATED} This rule has been deprecated${ |
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.
Yep, sorry, I'm not a native speaker of english
lib/rule-doc-notices.ts
Outdated
| : '.' | ||
| }${ | ||
| replacementRuleList.length > 0 | ||
| ? ` It was replaced by ${String(replacementRuleList)}.` |
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.
We want a comma-separative list with spaces.
| ? ` It was replaced by ${String(replacementRuleList)}.` | |
| ? ` It was replaced by ${replacementRuleList.join(', ')}.` |
I'm fixing in the existing code too: #731
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 could be improved even further, like
? ` It was replaced by ${
replacementRuleList.length > 1
? `${replacementsRuleLists.slice(0, -1).join(', ')} and ${String(replacementsRuleList.at(-1))}`
: replacementsRuleLists[0]
}.`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 open to that. Would want to extract a helper function for it.
lib/rule-doc-notices.ts
Outdated
| }${ | ||
| // use DeprecatedInfo#url to inform about the reasons | ||
| deprecatedInfo.url | ||
| ? `${EOL}${EOL}Read more at [${new URL(deprecatedInfo.url).hostname}](${deprecatedInfo.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.
It's an interesting idea to use the hostname, but I think I'd rather just use this, and keep everything related to deprecations on the same line.
| ? `${EOL}${EOL}Read more at [${new URL(deprecatedInfo.url).hostname}](${deprecatedInfo.url})` | |
| ? `[Read more](deprecatedInfo.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.
See this comment for desired format: #730 (comment)
lib/rule-doc-notices.ts
Outdated
| // warn and use fallback | ||
| console.warn( | ||
| [ | ||
| 'The two top-level properties `deprecated` and `replacedBy` are deprecated since eslint 9.21.0.', | ||
| 'Please consider using the new object type `DeprecatedInfo`.', | ||
| 'https://eslint.org/docs/latest/extend/rule-deprecation#-deprecatedinfo-type', | ||
| ].join('\n'), | ||
| ); | ||
|
|
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.
Actually, while it's a nice idea to warn about this, I'd rather use a new lint rule in eslint-plugin-eslint-plugin to do this:
eslint-doc-generator is not really intended to warn about deprecations or older styles. That's the job of eslint-plugin-eslint-plugin.
So I'd like to remove the warning.
| }); | ||
| }); | ||
|
|
||
| describe('with the old format', function () { |
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 we can remove this test as replacedBy is already tested and we're removing the warning.
lib/rule-doc-notices.ts
Outdated
| ruleName, | ||
| urlRuleDoc, | ||
| }) => { | ||
| // use object type `DeprecatedInfo` |
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.
We need to include deprecated.availableUntil (and test it).
| meta: { | ||
| docs: { description: 'Description.' }, | ||
| deprecated: { | ||
| message: 'This rule is outdated', |
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.
We need to make sure deprecated.message is displayed.
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 didn't include the message because it's an unknown value and could break the flow of the notices. It seems like a design choice to me. If all fields are used and there are replacement rules it could become cluttered and confusing depending on the position of the message e.g.
❌ This rule is deprecated since v1.0.0 but will be available until v2.0.0. Here's a message from the plugin maintainer. It was replaced by \`other-plugin/no-unused-vars\` and \`another-plugin/no-unused-vars\`. Read more.That's also the reason why I've added a line break before the 'Read more' url. My idea would be to show the message if there aren't any other properties or replacement rules or put message and url on a new line in case both of them are set.
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've just noticed that the RFC has a quite specific example. I assume the code should be aligned to it, right?
❌ This rule has been deprecated since v1.0.0 but will be available until v2.0.0.
It was replaced byreplacedByInfo[0]#rule#namefromreplacedByInfo[0]#plugin#name
replacedByInfo[0]#messageRead more.
It was also replaced byreplacedByInfo[1]#rule#namefromreplacedByInfo[1]#plugin#name
replacedByInfo[1]#messageRead more.
deprecatedInfo#message. Read more.
Unfortunately it's not quite clear how the properties message and url of the ReplacedByInfo type should be included.
Handling multiple replacements is a bit cumbersome.
So it's more like
const replacementRuleList = (deprecatedInfo.replacedBy ?? []).map(replacedByInfoToNotice)
function replacedByInfoToNotice(info, i, arr) { /* ... */ };
The indices could be used to check if line breaks or transition words are required.
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 sample in the RFC is very rough (I wrote it originally) and we don't have to follow it exactly.
Here's what I'm thinking the format should be.
The first sentence is a variation of:
- ❌ This rule is deprecated.
- ❌ This rule is deprecated and will be available until v2.0.0.
- With
availableUntil
- With
- ❌ This rule has been deprecated since v1.0.0.
- With
deprecatedSince
- With
- ❌ This rule has been deprecated since v1.0.0 and will be available until v2.0.0.
- With
deprecatedSinceandavailableUntil
- With
- ❌ This rule is deprecated.
- Link on
deprecatedwhenurlis provided
- Link on
The second sentence is optional replacement info:
- It was replaced by
foo,bar, andbaz.- CSV format when multiple replacements
- It was replaced by
other-plugin/no-unused-vars.- Display plugin/rule format when available
- It was replaced by
foo,bar, andbaz.- Includes a link on the rule name when we can determine one (e.g.
getLinkToRulewill try to generate a local link when possible) or otherwise a link to the rule URL provided
- Includes a link on the rule name when we can determine one (e.g.
- It was replaced by
foo(custom message),bar(custom message), andbaz(custom message).- Custom message for each replacement in parentheses.
- It was replaced by
foo(custom message) (read more).- Read more link in parentheses after rule name / custom message.
The third sentence is the optional custom message.
- Custom message about overall deprecation.
Putting that all together on a single line:
- ❌ This rule has been deprecated since v1.0.0 and will be available until v2.0.0. It was replaced by
foo(custom message about this replacement) (read more). Custom message about overall deprecation.
I think this looks good on a single line. And most of the time, we won't have all the info filled in anyway.
| ); | ||
|
|
||
| const replacementRuleList = (replacedBy ?? []).map((replacementRuleName) => | ||
| getLinkToRule( |
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.
To me it seems that the DeprecatedInfo has shifted the responsibility of correct links/data of deprecated rules towards the maintainers of eslint plugins. I think eslint-doc-generator should just display the data of the deprecated rules which makes the usage of getLinkToRule in [NOTICE_TYPE.DEPRECATED] obsolete. Please let me know if this is the desired approach.
To your question about this, I think I would still like to automatically fill in links for rules where we can. Including URLs in rule definitions is burdensome, and while rules theoretically could be responsible for it now, I assume many will omit it. However, we can try to use getLinkToRule as a follow-up if you'd like, doesn't have to be in the first version.
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 like to give it a shot. But first let me check if I understood it correctly and considered the relevant cases:
In case a user of eslint-doc-generator has set ReplacedByInfo#url we're good to go. Otherwise we'd use getLinkToRule as a fallback. The one case that's bothering me is when only ReplacedByInfo#name is set. If it has a plugin prefix we should compare it with the argument pluginPrefix to determine if it's an internal or external rule because we can't rely on ReplacedByInfo#plugin being set. If the name does not have a plugin prefix, getLinkToRule is used to determine if it is a built-in eslint rule or a internal plugin rule, right?
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 totally sure. getLinkToRule might not handle every situation here.
If any scenarios are ambiguous or overly-complicated, we can just omit the link in them and consider as a follow-up.
No need for a deprecation warning in the readme. |
DeprecatedInfoDeprecatedInfo format for rule meta.deprecated
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.
Great start!
But the in-house 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.
But the in-house example require-baz.md should be updated, right?
Regarding this, let's update that example file to show this notice:
❌ This rule has been deprecated since v1.0.0 and will be available until v2.0.0. It was replaced by test/prefer-bar (custom message about this replacement) (read more). Custom message about overall deprecation.
|
@bmish At the moment I feel like it's worthwile to split |
|
@error-four-o-four thanks for your work so far. Yes, I won't be surprised if we need helper functions and comprehensive tests if |
Fixes #512.
Hey there,
This PR implements support for the new
DeprecatedInfoobject type for rule deprecation, as introduced in ESLint 9.21.0 and updates the dependencies to require@typescript-eslint/utils@^8.34.0and related packages.An example of its usage can be found in @typescirpt-eslint/typedef and @typescript-eslint/sort-type-constituents
The usage of
DeprecatedInfois preferred over the top-level propertiesdeprecated: booleanandreplacedBy: readonly string[] | undefined. The latter are kept in order to support previous versions. Additionally a test is added to ensure a warning is shown when these are used. Thereforeconsole.warnis mocked injest.setup.cjsto suppress deprecation warnings during test runs.I considered adding a deprecation warning in the
README.mdbut I'm uncertain about where to add it.To me it seems that the
DeprecatedInfohas shifted the responsibility of correct links/data of deprecated rules towards the maintainers of eslint plugins. I thinkeslint-doc-generatorshould just display the data of the deprecated rules which makes the usage ofgetLinkToRulein[NOTICE_TYPE.DEPRECATED]obsolete. Please let me know if this is the desired approach.