Skip to content

Conversation

error-four-o-four
Copy link

@error-four-o-four error-four-o-four commented Jun 15, 2025

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 properties deprecated: boolean and replacedBy: 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. Therefore console.warn is mocked in jest.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 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.

@bmish bmish added the enhancement New feature or request label Jun 15, 2025
fixable: boolean;
hasSuggestions: boolean;
urlConfigs?: string;
deprecatedInfo: boolean | DeprecatedInfo | undefined;
Copy link
Owner

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

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

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.

)
.filter((rule): rule is string => typeof rule === 'string');

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

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

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

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

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

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

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

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

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)

Comment on lines 230 to 238
// 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

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 () {
Copy link
Owner

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.

ruleName,
urlRuleDoc,
}) => {
// use object type `DeprecatedInfo`
Copy link
Owner

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',
Copy link
Owner

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.

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.

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.

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.

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 by replacedByInfo[0]#rule#name from replacedByInfo[0]#plugin#name
replacedByInfo[0]#message Read more.
It was also replaced by replacedByInfo[1]#rule#name from replacedByInfo[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.

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.

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
  • ❌ This rule has been deprecated since v1.0.0.
    • With deprecatedSince
  • ❌ This rule has been deprecated since v1.0.0 and will be available until v2.0.0.
    • With deprecatedSince and availableUntil
  • ❌ This rule is deprecated.
    • Link on deprecated when url is provided

The second sentence is optional replacement info:

  • It was replaced by foo, bar, and baz.
    • 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, and baz.
    • 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
  • It was replaced by foo (custom message), bar (custom message), and baz (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(
Copy link
Owner

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

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

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.

@bmish
Copy link
Owner

bmish commented Jun 15, 2025

I considered adding a deprecation warning in the README.md but I'm uncertain about where to add it.

No need for a deprecation warning in the readme.

@bmish bmish changed the title chore: adopt type DeprecatedInfo Support new DeprecatedInfo format for rule meta.deprecated Jun 15, 2025
Copy link
Owner

@bmish bmish left a comment

Choose a reason for hiding this comment

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

Great start!

@error-four-o-four
Copy link
Author

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

No need for a deprecation warning in the readme.

But the in-house example require-baz.md should be updated, right?

Copy link
Owner

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.

@error-four-o-four
Copy link
Author

@bmish
Just wanted to let you know that I'm still working on this and check if it's going in the right direction. It's still a WIP because the getLinkToRule logic doesn't work correctly yet (see @todo comment :118)

At the moment I feel like it's worthwile to split getLinkToRule into smaller helper functions to reuse them where it could be neccessary.

@bmish
Copy link
Owner

bmish commented Jul 13, 2025

@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 getLinkToRule is getting complicated. If the PR is getting too complicated, feel free to use a simple approach for now and follow-up with improvements later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support additional rule metadata for deprecations
2 participants