Skip to content

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.

Sign up for GitHub

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

Open
error-four-o-four wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Support new DeprecatedInfo format for rule meta.deprecated #730

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8c38608
feat: implement `DeprecatedInfo`
error-four-o-four Jun 15, 2025
4436953
test: add tests for `DeprecatedInfo`
error-four-o-four Jun 15, 2025
a0f8b6f
Merge branch 'main' into chore/metadata-issue-512
error-four-o-four Jun 15, 2025
783837b
Merge branch 'main' into chore/metadata-issue-512
error-four-o-four Jun 16, 2025
22be021
Merge branch 'main' into chore/metadata-issue-512
error-four-o-four Jun 23, 2025
d90c3bb
cleanup: remove the warning
error-four-o-four Jun 16, 2025
479b0d8
fixing as per suggestions
error-four-o-four Jul 9, 2025
ede4a08
test: update deprecation tests
error-four-o-four Jul 9, 2025
4903b0d
docs: update in-house example `require-baz`
error-four-o-four Jul 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 45 additions & 3 deletions lib/rule-doc-notices.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
import { findConfigEmoji, getConfigsForRule } from './plugin-configs.js';
import {
RuleModule,
DeprecatedInfo,
Plugin,
ConfigsToRules,
ConfigEmojis,
Expand Down Expand Up @@ -104,6 +105,7 @@ const RULE_NOTICES: {
fixable: boolean;
hasSuggestions: boolean;
urlConfigs?: string;
deprecatedInfo: boolean | DeprecatedInfo | undefined;
Copy link
Owner

@bmish bmish Jun 15, 2025

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?

Copy link
Author

@error-four-o-four error-four-o-four Jun 16, 2025

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

Copy link
Owner

@bmish bmish Jun 16, 2025

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.

replacedBy: readonly string[] | undefined;
plugin: Plugin;
pluginPrefix: string;
Expand Down Expand Up @@ -187,8 +189,8 @@ const RULE_NOTICES: {
return `${emojis.join('')} ${sentences}`;
},

// Deprecated notice has optional "replaced by" rules list.
[NOTICE_TYPE.DEPRECATED]: ({
deprecatedInfo,
replacedBy,
plugin,
pluginPrefix,
Expand All @@ -197,6 +199,43 @@ const RULE_NOTICES: {
ruleName,
urlRuleDoc,
}) => {
// use object type `DeprecatedInfo`
Copy link
Owner

@bmish bmish Jun 15, 2025

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

if (typeof deprecatedInfo === 'object') {
const replacementRuleList = (deprecatedInfo.replacedBy ?? [])
.map(({ rule }) =>
rule && rule.name
? rule.url
? `[\`${rule.name}\`](${rule.url})`
: `\`${rule.name}\``
: undefined,
)
.filter((rule): rule is string => typeof rule === 'string');

return `${EMOJI_DEPRECATED} This rule is deprecated${
Copy link
Owner

@bmish bmish Jun 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return `${EMOJI_DEPRECATED} This rule is deprecated${
return `${EMOJI_DEPRECATED} This rule has been deprecated${

Copy link
Author

@error-four-o-four error-four-o-four Jun 16, 2025

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

deprecatedInfo.deprecatedSince
? ` since v${deprecatedInfo.deprecatedSince}.`
: '.'
}${
replacementRuleList.length > 0
? ` It was replaced by ${String(replacementRuleList)}.`
Copy link
Owner

@bmish bmish Jun 15, 2025

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.

Suggested change
? ` It was replaced by ${String(replacementRuleList)}.`
? ` It was replaced by ${replacementRuleList.join(', ')}.`

I'm fixing in the existing code too: #731

Copy link
Author

@error-four-o-four error-four-o-four Jun 16, 2025 •
edited
Loading

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]
		}.`

Copy link
Owner

@bmish bmish Jun 16, 2025

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.

: ''
}${
// use DeprecatedInfo#url to inform about the reasons
deprecatedInfo.url
? `${EOL}${EOL}Read more at [${new URL(deprecatedInfo.url).hostname}](${deprecatedInfo.url})`
Copy link
Owner

@bmish bmish Jun 15, 2025

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.

Suggested change
? `${EOL}${EOL}Read more at [${new URL(deprecatedInfo.url).hostname}](${deprecatedInfo.url})`
? `[Read more](deprecatedInfo.url).`

Copy link
Owner

@bmish bmish Jun 27, 2025

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)

: ''
}`;
}

// 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'),
);

Copy link
Owner

@bmish bmish Jun 15, 2025 •
edited
Loading

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.

const replacementRuleList = (replacedBy ?? []).map((replacementRuleName) =>
getLinkToRule(
Copy link
Owner

@bmish bmish Jun 15, 2025

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.

Copy link
Author

@error-four-o-four error-four-o-four Jun 16, 2025 •
edited
Loading

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?

Copy link
Owner

@bmish bmish Jun 27, 2025

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.

replacementRuleName,
Expand All @@ -210,6 +249,7 @@ const RULE_NOTICES: {
urlRuleDoc,
),
);

return `${EMOJI_DEPRECATED} This rule is deprecated.${
replacedBy && replacedBy.length > 0
? ` It was replaced by ${String(replacementRuleList)}.`
Expand Down Expand Up @@ -277,7 +317,7 @@ function getNoticesForRule(
configsError.length > 0 ||
configsWarn.length > 0 ||
configsOff.length > 0,
[NOTICE_TYPE.DEPRECATED]: rule.meta?.deprecated || false,
[NOTICE_TYPE.DEPRECATED]: Boolean(rule.meta?.deprecated) || false,
[NOTICE_TYPE.DESCRIPTION]: Boolean(rule.meta?.docs?.description) || false,

// Fixable/suggestions.
Expand Down Expand Up @@ -359,6 +399,7 @@ function getRuleNoticeLines(
configsOff,
ruleDocNotices,
);

let noticeType: keyof typeof notices;

for (noticeType in notices) {
Expand Down Expand Up @@ -392,7 +433,8 @@ function getRuleNoticeLines(
fixable: Boolean(rule.meta?.fixable),
hasSuggestions: Boolean(rule.meta?.hasSuggestions),
urlConfigs,
replacedBy: rule.meta?.replacedBy,
deprecatedInfo: rule.meta?.deprecated,
replacedBy: rule.meta?.replacedBy, // eslint-disable-line @typescript-eslint/no-deprecated
plugin,
pluginPrefix,
pathPlugin,
Expand Down
2 changes: 2 additions & 0 deletions lib/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ export type Rules = TSESLint.Linter.RulesRecord;

export type RuleSeverity = TSESLint.Linter.RuleLevel;

export type DeprecatedInfo = TSESLint.DeprecatedInfo;

export type Config = TSESLint.Linter.Config;

export type Plugin = TSESLint.Linter.Plugin;
Expand Down
Loading