feat: report malformed rule deprecation metadata#147
Conversation
| - `message`, `url`, and `deprecatedSince` must be strings when present. | ||
| - `availableUntil` must be a string or `null` when present. | ||
| - `replacedBy` must be an array when present. | ||
| - No other top-level `DeprecatedInfo` properties are allowed. |
There was a problem hiding this comment.
Is there any reason to disallow unknown properties when ESLint does not use them.
This would allow plugin authors to add additional metadata for their use case(see the mentioned drawbacks).
There was a problem hiding this comment.
DeprecatedInfo does not allow unknown properties in its type definition. If someone writes their rule in TypeScript, trying to add a custom property to this object will result in a compiler error. Validating it strictly in RuleTester simply brings the runtime behavior in line with what the TypeScript types already enforce. Furthermore, the Rule Deprecation documentation does not mention that this object can be extended with custom metadata.
There was a problem hiding this comment.
But TypeScript also allows overwriting the types of external packages which can be used to extend the type, which would then not be possible for the rule tester.
Further I think using the types as a guideline is misguided in thiscase as the types allow every defined "subobject" to be empty which I would think the validator should report.
There was a problem hiding this comment.
Because this section is meant to be used by ESLint itself, I'm fine with disallowing unknown keys. There are plenty of other places inside of meta where people can add custom metadata for other purposes.
nzakas
left a comment
nzakas
left a comment
There was a problem hiding this comment.
Overall LGTM. Just left a couple of notes for clarification.
| - `message`, `url`, and `deprecatedSince` must be strings when present. | ||
| - `availableUntil` must be a string or `null` when present. | ||
| - `replacedBy` must be an array when present. | ||
| - No other top-level `DeprecatedInfo` properties are allowed. |
There was a problem hiding this comment.
Because this section is meant to be used by ESLint itself, I'm fine with disallowing unknown keys. There are plenty of other places inside of meta where people can add custom metadata for other purposes.
|
|
||
| ## Drawbacks | ||
|
|
||
| This is a breaking change for rule packages whose tests currently pass with malformed deprecation metadata. Some projects may also be storing custom, nonstandard fields under `meta.deprecated`; those projects will need to remove those fields or move them elsewhere. |
There was a problem hiding this comment.
Worth noting that we've decided breaking changes to RuleTester are acceptable outside of major version bumps because they affect plugin developers only and not end users. The utility is meant to ensure compatibility with the version of ESLint being used.
There was a problem hiding this comment.
I don't recall us deciding that, can you provide a link (to a TSC meeting where we discussed this?). As far as I'm aware, all breaking changes to RuleTester were always considered breaking changes to ESLint as well and thus required a major version bump.
Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com>
|
|
||
| ## Motivation | ||
|
|
||
| Rule deprecation metadata is used by ESLint to populate `usedDeprecatedRules` and by integrations to explain replacement rules. Today, malformed `meta.deprecated` values can pass through rule tests unnoticed, which allows invalid metadata to reach users and downstream tools. `RuleTester` already catches common rule authoring mistakes such as invalid option schemas, missing `meta.fixable`, and missing `meta.hasSuggestions`; deprecation metadata should receive the same early feedback. |
There was a problem hiding this comment.
| Rule deprecation metadata is used by ESLint to populate `usedDeprecatedRules` and by integrations to explain replacement rules. Today, malformed `meta.deprecated` values can pass through rule tests unnoticed, which allows invalid metadata to reach users and downstream tools. `RuleTester` already catches common rule authoring mistakes such as invalid option schemas, missing `meta.fixable`, and missing `meta.hasSuggestions`; deprecation metadata should receive the same early feedback. | |
| Rule deprecation metadata is used by ESLint to populate `LintResult#usedDeprecatedRules` and by integrations to explain replacement rules. Today, malformed `meta.deprecated` values can pass through rule tests unnoticed, which allows invalid metadata to reach users and downstream tools. `RuleTester` already catches common rule authoring mistakes such as invalid option schemas, missing `meta.fixable`, and missing `meta.hasSuggestions`; deprecation metadata should receive the same early feedback. |
To be more specific.
| - `message`, `url`, and `deprecatedSince` must be strings when present. | ||
| - `availableUntil` must be a string or `null` when present. |
There was a problem hiding this comment.
For semver-format strings, perhaps we could also check that they don't start with "v"?
It should also be possible to check if the string is valid semver, but I'm not sure if we would want to add a dependency for this purpose.
4 participants
Summary
RuleTestershould validate the shape ofmeta.deprecatedon the rule under test and throw an assertion error when the value does not match the documentedboolean | DeprecatedInfocontract.Related Issues
eslint/eslint#20603