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
feat!: Set default schema: []
, drop support for function-style rules
#17792
Changes from 20 commits
9d924e3
c7a0b42
0964578
b37aeb4
1658e6f
d868180
47c060b
cd2caa0
3cc25bb
f7c4f8d
1671198
e3ba846
91fe762
5d6f96f
b3fa339
dabe3ef
e078476
78145ec
d268d81
a92d4ab
27b4448
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -317,6 +317,14 @@ function throwForbiddenMethodError(methodName, prototype) { | |
}; | ||
} | ||
|
||
const metaSchemaDescription = ` | ||
\t- If the rule has options, set \`meta.schema\` to an array or non-empty object to enable options validation. | ||
\t- If the rule doesn't have options, omit \`meta.schema\` to enforce that no options can be passed to the rule. | ||
\t- You can also set \`meta.schema\` to \`false\` to opt-out of options validation (not recommended). | ||
|
||
\thttps://eslint.org/docs/latest/extend/custom-rules#options-schemas | ||
`; | ||
|
||
//------------------------------------------------------------------------------ | ||
// Public Interface | ||
//------------------------------------------------------------------------------ | ||
|
@@ -490,13 +498,13 @@ class FlatRuleTester { | |
/** | ||
* Adds a new rule test to execute. | ||
* @param {string} ruleName The name of the rule to run. | ||
* @param {Function | Rule} rule The rule to test. | ||
* @param {Rule} rule The rule to test. | ||
* @param {{ | ||
* valid: (ValidTestCase | string)[], | ||
* invalid: InvalidTestCase[] | ||
* }} test The collection of tests to run. | ||
* @throws {TypeError|Error} If non-object `test`, or if a required | ||
* scenario of the given type is missing. | ||
* @throws {TypeError|Error} If `rule` is not an object with a `create` method, | ||
* or if non-object `test`, or if a required scenario of the given type is missing. | ||
* @returns {void} | ||
*/ | ||
run(ruleName, rule, test) { | ||
|
@@ -507,6 +515,10 @@ class FlatRuleTester { | |
linter = this.linter, | ||
ruleId = `rule-to-test/${ruleName}`; | ||
|
||
if (!rule || typeof rule !== "object" || typeof rule.create !== "function") { | ||
throw new TypeError("Rule must be an object with a `create` method"); | ||
} | ||
|
||
if (!test || typeof test !== "object") { | ||
throw new TypeError(`Test Scenarios for rule ${ruleName} : Could not find test scenario object`); | ||
} | ||
|
@@ -560,7 +572,7 @@ class FlatRuleTester { | |
|
||
// freezeDeeply(context.languageOptions); | ||
|
||
return (typeof rule === "function" ? rule : rule.create)(context); | ||
return rule.create(context); | ||
} | ||
}) | ||
} | ||
|
@@ -652,7 +664,31 @@ class FlatRuleTester { | |
} | ||
}); | ||
|
||
const schema = getRuleOptionsSchema(rule); | ||
let schema; | ||
|
||
try { | ||
schema = getRuleOptionsSchema(rule); | ||
} catch (err) { | ||
err.message += metaSchemaDescription; | ||
throw err; | ||
} | ||
|
||
/* | ||
* Check and throw an error if the schema is an empty object (`schema:{}`), because such schema | ||
* doesn't validate or enforce anything and is therefore considered a possible error. If the intent | ||
* was to skip options validation, `schema:false` should be set instead (explicit opt-out). | ||
* | ||
* For this purpose, a schema object is considered empty if it doesn't have any own enumerable string-keyed | ||
* properties. While `ajv.compile()` does use enumerable properties from the prototype chain as well, | ||
* it caches compiled schemas by serializing only own enumerable properties, so it's generally not a good idea | ||
* to use inherited properties in schemas because schemas that differ only in inherited properties would end up | ||
* having the same cache entry that would be correct for only one of them. | ||
* | ||
* At this point, `schema` can only be an object or `null`. | ||
*/ | ||
if (schema && Object.keys(schema).length === 0) { | ||
throw new Error(`\`schema: {}\` is a no-op${metaSchemaDescription}`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Disallowing I'm fine with this one check for now, but just want to ask if any other obvious/common no-op schemas come to mind? In the RFC, I mentioned There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there a related discussion in ajv repo? it could benefit all the dependents (not just eslint) - if it could be supported by ajv. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did a brief search and didn't immediately find a relevant discussion in in ajv. But I have included a lot more context in this issue discussing a potential lint rule: Overall, we should probably just allow any schema in ESLint except extremely common and obvious mistakes like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Newer versions of Ajv have some built-in validations, but there were backwards compatibility issues when we tried to switch to Ajv 7+ (#13911). However, I believe I agree that for now RuleTester should only check for the common and obvious mistake |
||
} | ||
|
||
/* | ||
* Setup AST getters. | ||
|
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.
In general, should we avoid deleting pages to avoid breaking links, and just add a placeholder page linking to the replacement page?
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.
Good idea, I've added back and updated this document in d14e2b5.
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 looks like we also could do a redirect like this: eslint/eslint.org#499
Discussion here: #17840 (comment)
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.
Maybe, but this is a slightly different situation so I'm not sure.
For
configuration-files-new
, its content is moved toconfiguration-files
, so redirecting makes the most sense.For
custom-rules-deprecated
, its content is not moved tocustom-rules
but deleted, so info that it no longer applies in ESLint 9+ might be more helpful.