From 802e9262b5cabc0872afd654ce1c9d0dc331f76b Mon Sep 17 00:00:00 2001 From: Colin Chang Date: Tue, 30 Oct 2018 12:39:19 -0400 Subject: [PATCH] Update: Warn for deprecation in Node output (fixes #7443) (#10953) * Fix: Warn for deprecation in Node output (fixes #7443) * Reduce complexity of createRuleWarnings() * Specify the usedDeprecatedRules object properties in the docs * Add safety checks for createRuleWarnings() * Document replacedBy in working-with-rules * Move meta.docs.replacedBy to meta.replacedBy --- Makefile.js | 2 +- docs/developer-guide/nodejs-api.md | 10 +- docs/developer-guide/working-with-rules.md | 2 + lib/cli-engine.js | 35 ++++- lib/rules/indent-legacy.js | 3 +- lib/rules/lines-around-directive.js | 4 +- lib/rules/newline-after-var.js | 5 +- lib/rules/newline-before-return.js | 4 +- lib/rules/no-catch-shadow.js | 5 +- lib/rules/no-native-reassign.js | 3 +- lib/rules/no-negated-in-lhs.js | 3 +- lib/rules/no-spaced-func.js | 3 +- lib/rules/prefer-reflect.js | 3 +- tests/lib/cli-engine.js | 155 ++++++++++++--------- 14 files changed, 155 insertions(+), 82 deletions(-) diff --git a/Makefile.js b/Makefile.js index 473ffdf5887..2a5c7ab8d69 100644 --- a/Makefile.js +++ b/Makefile.js @@ -226,7 +226,7 @@ function generateRuleIndexPage(basedir) { if (rule.meta.deprecated) { categoriesData.deprecated.rules.push({ name: basename, - replacedBy: rule.meta.docs.replacedBy || [] + replacedBy: rule.meta.replacedBy || [] }); } else { const output = { diff --git a/docs/developer-guide/nodejs-api.md b/docs/developer-guide/nodejs-api.md index 8dbccc7afb1..0ff1e04ef2c 100644 --- a/docs/developer-guide/nodejs-api.md +++ b/docs/developer-guide/nodejs-api.md @@ -424,7 +424,8 @@ The return value is an object containing the results of the linting operation. H errorCount: 1, warningCount: 0, fixableErrorCount: 1, - fixableWarningCount: 0 + fixableWarningCount: 0, + usedDeprecatedRules: [] } ``` @@ -482,6 +483,7 @@ var report = cli.executeOnFiles(["myfile.js", "lib/"]); warningCount: 0, fixableErrorCount: 1, fixableWarningCount: 0, + usedDeprecatedRules: [] } ``` @@ -513,6 +515,7 @@ If the operation ends with a parsing error, you will get a single message for th warningCount: 0, fixableErrorCount: 0, fixableWarningCount: 0, + usedDeprecatedRules: [] } ``` @@ -524,7 +527,10 @@ The top-level report object has a `results` array containing all linting results * `source` - The source code for the given file. This property is omitted if this file has no errors/warnings or if the `output` property is present. * `output` - The source code for the given file with as many fixes applied as possible, so you can use that to rewrite the files if necessary. This property is omitted if no fix is available. -The top-level report object also has `errorCount` and `warningCount` which give the exact number of errors and warnings respectively on all the files. +The top-level report object also has `errorCount` and `warningCount` which give the exact number of errors and warnings respectively on all the files. Additionally, `usedDeprecatedRules` signals any deprecated rules used and their replacement (if available). Specifically, it is array of objects with properties like so: + +* `ruleId` - The name of the rule (e.g. `indent-legacy`). +* `replacedBy` - An array of rules that replace the deprecated rule (e.g. `["indent"]`). Once you get a report object, it's up to you to determine how to output the results. Fixes will not be automatically applied to the files, even if you set `fix: true` when constructing the `CLIEngine` instance. To apply fixes to the files, call [`outputFixes`](#cliengineoutputfixes). diff --git a/docs/developer-guide/working-with-rules.md b/docs/developer-guide/working-with-rules.md index 8b5c0d5acf9..85b8c25ea38 100644 --- a/docs/developer-guide/working-with-rules.md +++ b/docs/developer-guide/working-with-rules.md @@ -66,6 +66,8 @@ The source file for a rule exports an object with the following properties. * `deprecated` (boolean) indicates whether the rule has been deprecated. You may omit the `deprecated` property if the rule has not been deprecated. +* `replacedBy` (array) in the case of a deprecated rule, specifies replacement rule(s) + `create` (function) returns an object with methods that ESLint calls to "visit" nodes while traversing the abstract syntax tree (AST as defined by [ESTree](https://github.com/estree/estree)) of JavaScript code: * if a key is a node type or a [selector](./selectors.md), ESLint calls that **visitor** function while going **down** the tree diff --git a/lib/cli-engine.js b/lib/cli-engine.js index 5b52459ac83..1545bf4c015 100644 --- a/lib/cli-engine.js +++ b/lib/cli-engine.js @@ -19,6 +19,7 @@ const fs = require("fs"), path = require("path"), defaultOptions = require("../conf/default-cli-options"), Linter = require("./linter"), + lodash = require("lodash"), IgnoredPaths = require("./ignored-paths"), Config = require("./config"), LintResultCache = require("./util/lint-result-cache"), @@ -272,6 +273,35 @@ function createIgnoreResult(filePath, baseDir) { }; } +/** + * Produces rule warnings (i.e. deprecation) from configured rules + * @param {Object} rules - Rules configured + * @param {Map} loadedRules - Map of loaded rules + * @returns {Object} Contains rule warnings + * @private + */ +function createRuleWarnings(rules, loadedRules) { + const ruleWarnings = { usedDeprecatedRules: [] }; + + if (rules) { + Object.keys(rules).forEach(name => { + const loadedRule = loadedRules.get(name); + + if (loadedRule.meta && loadedRule.meta.deprecated) { + const deprecatedRule = { ruleId: name }; + const replacedBy = lodash.get(loadedRule, "meta.replacedBy", []); + + if (replacedBy.every(newRule => lodash.isString(newRule))) { + deprecatedRule.replacedBy = replacedBy; + } + + ruleWarnings.usedDeprecatedRules.push(deprecatedRule); + } + }); + } + + return ruleWarnings; +} /** * Checks if the given message is an error message. @@ -555,6 +585,8 @@ class CLIEngine { const stats = calculateStatsPerRun(results); + const ruleWarnings = createRuleWarnings(this.options.rules, this.getRules()); + debug(`Linting complete in: ${Date.now() - startTime}ms`); return { @@ -562,7 +594,8 @@ class CLIEngine { errorCount: stats.errorCount, warningCount: stats.warningCount, fixableErrorCount: stats.fixableErrorCount, - fixableWarningCount: stats.fixableWarningCount + fixableWarningCount: stats.fixableWarningCount, + usedDeprecatedRules: ruleWarnings.usedDeprecatedRules }; } diff --git a/lib/rules/indent-legacy.js b/lib/rules/indent-legacy.js index e6dc92163b0..c3858ba753f 100644 --- a/lib/rules/indent-legacy.js +++ b/lib/rules/indent-legacy.js @@ -25,12 +25,13 @@ module.exports = { description: "enforce consistent indentation", category: "Stylistic Issues", recommended: false, - replacedBy: ["indent"], url: "https://eslint.org/docs/rules/indent-legacy" }, deprecated: true, + replacedBy: ["indent"], + fixable: "whitespace", schema: [ diff --git a/lib/rules/lines-around-directive.js b/lib/rules/lines-around-directive.js index e8d613a41f0..2ed72942d46 100644 --- a/lib/rules/lines-around-directive.js +++ b/lib/rules/lines-around-directive.js @@ -18,7 +18,6 @@ module.exports = { description: "require or disallow newlines around directives", category: "Stylistic Issues", recommended: false, - replacedBy: ["padding-line-between-statements"], url: "https://eslint.org/docs/rules/lines-around-directive" }, schema: [{ @@ -42,7 +41,8 @@ module.exports = { ] }], fixable: "whitespace", - deprecated: true + deprecated: true, + replacedBy: ["padding-line-between-statements"] }, create(context) { diff --git a/lib/rules/newline-after-var.js b/lib/rules/newline-after-var.js index 62ccb3db29e..df2ad7620e7 100644 --- a/lib/rules/newline-after-var.js +++ b/lib/rules/newline-after-var.js @@ -22,7 +22,6 @@ module.exports = { description: "require or disallow an empty line after variable declarations", category: "Stylistic Issues", recommended: false, - replacedBy: ["padding-line-between-statements"], url: "https://eslint.org/docs/rules/newline-after-var" }, @@ -34,7 +33,9 @@ module.exports = { fixable: "whitespace", - deprecated: true + deprecated: true, + + replacedBy: ["padding-line-between-statements"] }, create(context) { diff --git a/lib/rules/newline-before-return.js b/lib/rules/newline-before-return.js index 5bc1f7031b0..e9de4bb1414 100644 --- a/lib/rules/newline-before-return.js +++ b/lib/rules/newline-before-return.js @@ -15,12 +15,12 @@ module.exports = { description: "require an empty line before `return` statements", category: "Stylistic Issues", recommended: false, - replacedBy: ["padding-line-between-statements"], url: "https://eslint.org/docs/rules/newline-before-return" }, fixable: "whitespace", schema: [], - deprecated: true + deprecated: true, + replacedBy: ["padding-line-between-statements"] }, create(context) { diff --git a/lib/rules/no-catch-shadow.js b/lib/rules/no-catch-shadow.js index f749490f31e..a9253dde047 100644 --- a/lib/rules/no-catch-shadow.js +++ b/lib/rules/no-catch-shadow.js @@ -22,11 +22,12 @@ module.exports = { description: "disallow `catch` clause parameters from shadowing variables in the outer scope", category: "Variables", recommended: false, - url: "https://eslint.org/docs/rules/no-catch-shadow", - replacedBy: ["no-shadow"] + url: "https://eslint.org/docs/rules/no-catch-shadow" }, deprecated: true, + replacedBy: ["no-shadow"], + schema: [], messages: { diff --git a/lib/rules/no-native-reassign.js b/lib/rules/no-native-reassign.js index b1064b0bb36..f24499b1491 100644 --- a/lib/rules/no-native-reassign.js +++ b/lib/rules/no-native-reassign.js @@ -16,12 +16,13 @@ module.exports = { description: "disallow assignments to native objects or read-only global variables", category: "Best Practices", recommended: false, - replacedBy: ["no-global-assign"], url: "https://eslint.org/docs/rules/no-native-reassign" }, deprecated: true, + replacedBy: ["no-global-assign"], + schema: [ { type: "object", diff --git a/lib/rules/no-negated-in-lhs.js b/lib/rules/no-negated-in-lhs.js index 7f08814c941..54252cdbccd 100644 --- a/lib/rules/no-negated-in-lhs.js +++ b/lib/rules/no-negated-in-lhs.js @@ -16,11 +16,12 @@ module.exports = { description: "disallow negating the left operand in `in` expressions", category: "Possible Errors", recommended: false, - replacedBy: ["no-unsafe-negation"], url: "https://eslint.org/docs/rules/no-negated-in-lhs" }, deprecated: true, + replacedBy: ["no-unsafe-negation"], + schema: [] }, diff --git a/lib/rules/no-spaced-func.js b/lib/rules/no-spaced-func.js index 42d1e4b243a..1f584f88bf2 100644 --- a/lib/rules/no-spaced-func.js +++ b/lib/rules/no-spaced-func.js @@ -16,12 +16,13 @@ module.exports = { description: "disallow spacing between function identifiers and their applications (deprecated)", category: "Stylistic Issues", recommended: false, - replacedBy: ["func-call-spacing"], url: "https://eslint.org/docs/rules/no-spaced-func" }, deprecated: true, + replacedBy: ["func-call-spacing"], + fixable: "whitespace", schema: [] }, diff --git a/lib/rules/prefer-reflect.js b/lib/rules/prefer-reflect.js index 765163e0eb3..bda827cb7a6 100644 --- a/lib/rules/prefer-reflect.js +++ b/lib/rules/prefer-reflect.js @@ -15,12 +15,13 @@ module.exports = { description: "require `Reflect` methods where applicable", category: "ECMAScript 6", recommended: false, - replacedBy: [], url: "https://eslint.org/docs/rules/prefer-reflect" }, deprecated: true, + replacedBy: [], + schema: [ { type: "object", diff --git a/tests/lib/cli-engine.js b/tests/lib/cli-engine.js index cac7671b197..92458b59af7 100644 --- a/tests/lib/cli-engine.js +++ b/tests/lib/cli-engine.js @@ -1375,6 +1375,33 @@ describe("CLIEngine", () => { assert.strictEqual(report.results[0].messages.length, 0); }); + it("should warn when deprecated rules are configured", () => { + engine = new CLIEngine({ + cwd: originalDir, + configFile: ".eslintrc.js", + rules: { "indent-legacy": 1 } + }); + + const report = engine.executeOnFiles(["lib/cli*.js"]); + + assert.deepStrictEqual( + report.usedDeprecatedRules, + [{ ruleId: "indent-legacy", replacedBy: ["indent"] }] + ); + }); + + it("should not warn when deprecated rules are not configured", () => { + engine = new CLIEngine({ + cwd: originalDir, + configFile: ".eslintrc.js", + rules: { indent: 1 } + }); + + const report = engine.executeOnFiles(["lib/cli*.js"]); + + assert.deepStrictEqual(report.usedDeprecatedRules, []); + }); + describe("Fix Mode", () => { it("should return fixed text on multiple files when in fix mode", () => { @@ -1407,70 +1434,68 @@ describe("CLIEngine", () => { const report = engine.executeOnFiles([path.resolve(fixtureDir, `${fixtureDir}/fixmode`)]); report.results.forEach(convertCRLF); - assert.deepStrictEqual(report, { - results: [ - { - filePath: fs.realpathSync(path.resolve(fixtureDir, "fixmode/multipass.js")), - messages: [], - errorCount: 0, - warningCount: 0, - fixableErrorCount: 0, - fixableWarningCount: 0, - output: "true ? \"yes\" : \"no\";\n" - }, - { - filePath: fs.realpathSync(path.resolve(fixtureDir, "fixmode/ok.js")), - messages: [], - errorCount: 0, - warningCount: 0, - fixableErrorCount: 0, - fixableWarningCount: 0 - }, - { - filePath: fs.realpathSync(path.resolve(fixtureDir, "fixmode/quotes-semi-eqeqeq.js")), - messages: [ - { - column: 9, - line: 2, - message: "Expected '===' and instead saw '=='.", - messageId: "unexpected", - nodeType: "BinaryExpression", - ruleId: "eqeqeq", - severity: 2 - } - ], - errorCount: 1, - warningCount: 0, - fixableErrorCount: 0, - fixableWarningCount: 0, - output: "var msg = \"hi\";\nif (msg == \"hi\") {\n\n}\n" - }, - { - filePath: fs.realpathSync(path.resolve(fixtureDir, "fixmode/quotes.js")), - messages: [ - { - column: 18, - line: 1, - endColumn: 21, - endLine: 1, - message: "'foo' is not defined.", - nodeType: "Identifier", - ruleId: "no-undef", - severity: 2 - } - ], - errorCount: 1, - warningCount: 0, - fixableErrorCount: 0, - fixableWarningCount: 0, - output: "var msg = \"hi\" + foo;\n" - } - ], - errorCount: 2, - warningCount: 0, - fixableErrorCount: 0, - fixableWarningCount: 0 - }); + assert.deepStrictEqual(report.results, [ + { + filePath: fs.realpathSync(path.resolve(fixtureDir, "fixmode/multipass.js")), + messages: [], + errorCount: 0, + warningCount: 0, + fixableErrorCount: 0, + fixableWarningCount: 0, + output: "true ? \"yes\" : \"no\";\n" + }, + { + filePath: fs.realpathSync(path.resolve(fixtureDir, "fixmode/ok.js")), + messages: [], + errorCount: 0, + warningCount: 0, + fixableErrorCount: 0, + fixableWarningCount: 0 + }, + { + filePath: fs.realpathSync(path.resolve(fixtureDir, "fixmode/quotes-semi-eqeqeq.js")), + messages: [ + { + column: 9, + line: 2, + message: "Expected '===' and instead saw '=='.", + messageId: "unexpected", + nodeType: "BinaryExpression", + ruleId: "eqeqeq", + severity: 2 + } + ], + errorCount: 1, + warningCount: 0, + fixableErrorCount: 0, + fixableWarningCount: 0, + output: "var msg = \"hi\";\nif (msg == \"hi\") {\n\n}\n" + }, + { + filePath: fs.realpathSync(path.resolve(fixtureDir, "fixmode/quotes.js")), + messages: [ + { + column: 18, + line: 1, + endColumn: 21, + endLine: 1, + message: "'foo' is not defined.", + nodeType: "Identifier", + ruleId: "no-undef", + severity: 2 + } + ], + errorCount: 1, + warningCount: 0, + fixableErrorCount: 0, + fixableWarningCount: 0, + output: "var msg = \"hi\" + foo;\n" + } + ]); + assert.strictEqual(report.errorCount, 2); + assert.strictEqual(report.warningCount, 0); + assert.strictEqual(report.fixableErrorCount, 0); + assert.strictEqual(report.fixableWarningCount, 0); }); it("should run autofix even if files are cached without autofix results", () => { @@ -2117,7 +2142,7 @@ describe("CLIEngine", () => { assert.deepStrictEqual(result, cachedResult, "the result is the same regardless of using cache or not"); // assert the file was not processed because the cache was used - assert.isFalse(spy.called, "the file was not loaded because it used the cache"); + assert.isFalse(spy.calledWith(file), "the file was not loaded because it used the cache"); }); it("should remember the files from a previous run and do not operate on then if not changed", () => {