-
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#name
fromreplacedByInfo[0]#plugin#name
replacedByInfo[0]#message
Read more.
It was also replaced byreplacedByInfo[1]#rule#name
fromreplacedByInfo[1]#plugin#name
replacedByInfo[1]#message
Read 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
deprecatedSince
andavailableUntil
- With
- ❌ This rule is deprecated.
- Link on
deprecated
whenurl
is 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.
getLinkToRule
will 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. |
DeprecatedInfo
DeprecatedInfo
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
DeprecatedInfo
object type for rule deprecation, as introduced in ESLint 9.21.0 and updates the dependencies to require@typescript-eslint/utils@^8.34.0
and related packages.An example of its usage can be found in @typescirpt-eslint/typedef and @typescript-eslint/sort-type-constituents
The usage of
DeprecatedInfo
is preferred over the top-level propertiesdeprecated: boolean
andreplacedBy: 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.warn
is mocked injest.setup.cjs
to suppress deprecation warnings during test runs.I considered adding a deprecation warning in the
README.md
but I'm uncertain about where to add it.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 thinkeslint-doc-generator
should just display the data of the deprecated rules which makes the usage ofgetLinkToRule
in[NOTICE_TYPE.DEPRECATED]
obsolete. Please let me know if this is the desired approach.