From c1fd6f54d92efe615bcae529006221e122dbe9e6 Mon Sep 17 00:00:00 2001 From: Teddy Katz Date: Wed, 30 Jan 2019 13:34:31 -0500 Subject: [PATCH] Chore: remove undocumented `Linter#rules` property (refs #9161) (#11335) This removes the undocumented `rules` property from `Linter` instances, as part of the effort to remove undocumented API surface from `Linter` (also see https://github.com/eslint/eslint/issues/9161). Config processing now exclusively uses `Linter`'s public API when defining rules. As a side-effect, the rule map utility in `lib/rules.js` no longer needs to access the filesystem, so we can remove the odd code generation logic from the browserify build. --- Makefile.js | 40 ++++--------------------- lib/cli-engine.js | 5 ++-- lib/config.js | 2 +- lib/config/plugins.js | 16 +++++++--- lib/linter.js | 12 ++++---- lib/rules.js | 45 ++-------------------------- tests/lib/config/config-validator.js | 23 +++++++------- tests/lib/config/plugins.js | 18 +++++------ tests/lib/load-rules.js | 25 ++++++++++++++++ tests/lib/rules.js | 43 -------------------------- 10 files changed, 76 insertions(+), 153 deletions(-) create mode 100644 tests/lib/load-rules.js diff --git a/Makefile.js b/Makefile.js index f38de4e3738..480edc218bf 100644 --- a/Makefile.js +++ b/Makefile.js @@ -117,27 +117,6 @@ function fileType(extension) { }; } -/** - * Generates a static file that includes each rule by name rather than dynamically - * looking up based on directory. This is used for the browser version of ESLint. - * @param {string} basedir The directory in which to look for code. - * @returns {void} - */ -function generateRulesIndex(basedir) { - let output = "module.exports = function() {\n"; - - output += " var rules = Object.create(null);\n"; - - find(`${basedir}rules/`).filter(fileType("js")).forEach(filename => { - const basename = path.basename(filename, ".js"); - - output += ` rules["${basename}"] = require("./rules/${basename}");\n`; - }); - - output += "\n return rules;\n};"; - output.to(`${basedir}load-rules.js`); -} - /** * Executes a command and returns the output instead of printing it to stdout. * @param {string} cmd The command string to execute. @@ -841,25 +820,16 @@ target.browserify = function() { mkdir(BUILD_DIR); } - // 2. copy files into temp directory - cp("-r", "lib/*", TEMP_DIR); - - // 3. delete the load-rules.js file - rm("-rf", `${TEMP_DIR}load-rules.js`); - - // 4. create new load-rule.js with hardcoded requires - generateRulesIndex(TEMP_DIR); - - // 5. browserify the temp directory - exec(`${getBinFile("browserify")} -x espree ${TEMP_DIR}linter.js -o ${BUILD_DIR}eslint.js -s eslint --global-transform [ babelify --presets [ es2015 ] ]`); + // 2. browserify the temp directory + exec(`${getBinFile("browserify")} -x espree lib/linter.js -o ${BUILD_DIR}eslint.js -s eslint --global-transform [ babelify --presets [ es2015 ] ]`); - // 6. Browserify espree + // 3. Browserify espree exec(`${getBinFile("browserify")} -r espree -o ${TEMP_DIR}espree.js --global-transform [ babelify --presets [ es2015 ] ]`); - // 7. Concatenate Babel polyfill, Espree, and ESLint files together + // 4. Concatenate Babel polyfill, Espree, and ESLint files together cat("./node_modules/babel-polyfill/dist/polyfill.js", `${TEMP_DIR}espree.js`, `${BUILD_DIR}eslint.js`).to(`${BUILD_DIR}eslint.js`); - // 8. remove temp directory + // 5. remove temp directory rm("-rf", TEMP_DIR); }; diff --git a/lib/cli-engine.js b/lib/cli-engine.js index ce9fe289b8c..d7de2592a16 100644 --- a/lib/cli-engine.js +++ b/lib/cli-engine.js @@ -29,7 +29,8 @@ const fs = require("fs"), hash = require("./util/hash"), ModuleResolver = require("./util/module-resolver"), naming = require("./util/naming"), - pkg = require("../package.json"); + pkg = require("../package.json"), + loadRules = require("./load-rules"); const debug = require("debug")("eslint:cli-engine"); const resolver = new ModuleResolver(); @@ -447,7 +448,7 @@ class CLIEngine { this.options.rulePaths.forEach(rulesdir => { debug(`Loading rules from ${rulesdir}`); - this.linter.rules.load(rulesdir, cwd); + this.linter.defineRules(loadRules(rulesdir, cwd)); }); } diff --git a/lib/config.js b/lib/config.js index abcb38b50d5..9c8438859ea 100644 --- a/lib/config.js +++ b/lib/config.js @@ -71,7 +71,7 @@ class Config { const options = providedOptions || {}; this.linterContext = linterContext; - this.plugins = new Plugins(linterContext.environments, linterContext.rules); + this.plugins = new Plugins(linterContext.environments, linterContext.defineRule.bind(linterContext)); this.options = options; this.ignore = options.ignore; diff --git a/lib/config/plugins.js b/lib/config/plugins.js index 37c3be497c4..f32cc26c33f 100644 --- a/lib/config/plugins.js +++ b/lib/config/plugins.js @@ -24,12 +24,12 @@ class Plugins { /** * Creates the plugins context * @param {Environments} envContext - env context - * @param {Rules} rulesContext - rules context + * @param {function(string, Rule): void} defineRule - Callback for when a plugin is defined which introduces rules */ - constructor(envContext, rulesContext) { + constructor(envContext, defineRule) { this._plugins = Object.create(null); this._environments = envContext; - this._rules = rulesContext; + this._defineRule = defineRule; } /** @@ -45,7 +45,15 @@ class Plugins { // load up environments and rules this._plugins[shortName] = plugin; this._environments.importPlugin(plugin, shortName); - this._rules.importPlugin(plugin, shortName); + + if (plugin.rules) { + Object.keys(plugin.rules).forEach(ruleId => { + const qualifiedRuleId = `${shortName}/${ruleId}`, + rule = plugin.rules[ruleId]; + + this._defineRule(qualifiedRuleId, rule); + }); + } } /** diff --git a/lib/linter.js b/lib/linter.js index d6d635f9f47..c3195cc6b87 100644 --- a/lib/linter.js +++ b/lib/linter.js @@ -758,6 +758,7 @@ function runRules(sourceCode, configuredRules, ruleMapper, parserOptions, parser const lastSourceCodes = new WeakMap(); const loadedParserMaps = new WeakMap(); +const ruleMaps = new WeakMap(); //------------------------------------------------------------------------------ // Public Interface @@ -772,9 +773,8 @@ module.exports = class Linter { constructor() { lastSourceCodes.set(this, null); loadedParserMaps.set(this, new Map()); + ruleMaps.set(this, new Rules()); this.version = pkg.version; - - this.rules = new Rules(); this.environments = new Environments(); } @@ -873,7 +873,7 @@ module.exports = class Linter { const sourceCode = lastSourceCodes.get(this); const commentDirectives = options.allowInlineConfig - ? getDirectiveComments(options.filename, sourceCode.ast, ruleId => this.rules.get(ruleId)) + ? getDirectiveComments(options.filename, sourceCode.ast, ruleId => ruleMaps.get(this).get(ruleId)) : { configuredRules: {}, enabledGlobals: {}, exportedVariables: {}, problems: [], disableDirectives: [] }; // augment global scope with declared global variables @@ -891,7 +891,7 @@ module.exports = class Linter { lintingProblems = runRules( sourceCode, configuredRules, - ruleId => this.rules.get(ruleId), + ruleId => ruleMaps.get(this).get(ruleId), parserOptions, parserName, settings, @@ -957,7 +957,7 @@ module.exports = class Linter { * @returns {void} */ defineRule(ruleId, ruleModule) { - this.rules.define(ruleId, ruleModule); + ruleMaps.get(this).define(ruleId, ruleModule); } /** @@ -976,7 +976,7 @@ module.exports = class Linter { * @returns {Map} All loaded rules */ getRules() { - return this.rules.getAllLoadedRules(); + return ruleMaps.get(this).getAllLoadedRules(); } /** diff --git a/lib/rules.js b/lib/rules.js index ee747311e79..d0f90951416 100644 --- a/lib/rules.js +++ b/lib/rules.js @@ -10,7 +10,6 @@ //------------------------------------------------------------------------------ const lodash = require("lodash"); -const loadRules = require("./load-rules"); const ruleReplacements = require("../conf/replacements").rules; const builtInRules = require("./built-in-rules-index"); @@ -60,7 +59,9 @@ function normalizeRule(rule) { class Rules { constructor() { this._rules = Object.create(null); - this.defineAll(builtInRules); + Object.keys(builtInRules).forEach(ruleId => { + this.define(ruleId, builtInRules[ruleId]); + }); } /** @@ -73,46 +74,6 @@ class Rules { this._rules[ruleId] = normalizeRule(ruleModule); } - /** - * Loads and registers all rules from passed rules directory. - * @param {string} [rulesDir] Path to rules directory, may be relative. Defaults to `lib/rules`. - * @param {string} cwd Current working directory - * @returns {void} - */ - load(rulesDir, cwd) { - const newRules = loadRules(rulesDir, cwd); - - this.defineAll(newRules); - } - - /** - * Pulls a Map of new rules to the defined ones of this instance. - * @param {Object} newRules Expects to have an object here that maps the rule ID to the rule definition. - * @returns {void} - */ - defineAll(newRules) { - Object.keys(newRules).forEach(ruleId => { - this.define(ruleId, newRules[ruleId]); - }); - } - - /** - * Registers all given rules of a plugin. - * @param {Object} plugin The plugin object to import. - * @param {string} pluginName The name of the plugin without prefix (`eslint-plugin-`). - * @returns {void} - */ - importPlugin(plugin, pluginName) { - if (plugin.rules) { - Object.keys(plugin.rules).forEach(ruleId => { - const qualifiedRuleId = `${pluginName}/${ruleId}`, - rule = plugin.rules[ruleId]; - - this.define(qualifiedRuleId, rule); - }); - } - } - /** * Access rule handler by id (file name). * @param {string} ruleId Rule id (file name). diff --git a/tests/lib/config/config-validator.js b/tests/lib/config/config-validator.js index b4cb2e41d8d..721533c1030 100644 --- a/tests/lib/config/config-validator.js +++ b/tests/lib/config/config-validator.js @@ -11,7 +11,8 @@ const assert = require("chai").assert, Linter = require("../../../lib/linter"), - validator = require("../../../lib/config/config-validator"); + validator = require("../../../lib/config/config-validator"), + Rules = require("../../../lib/rules"); const linter = new Linter(); //------------------------------------------------------------------------------ @@ -96,7 +97,7 @@ describe("Validator", () => { * @returns {{create: Function}} The loaded rule */ function ruleMapper(ruleId) { - return linter.getRules().get(ruleId); + return linter.getRules().get(ruleId) || new Rules().get(ruleId); } beforeEach(() => { @@ -403,12 +404,12 @@ describe("Validator", () => { describe("getRuleOptionsSchema", () => { it("should return null for a missing rule", () => { - assert.strictEqual(validator.getRuleOptionsSchema(linter.rules.get("non-existent-rule")), null); + assert.strictEqual(validator.getRuleOptionsSchema(ruleMapper("non-existent-rule")), null); }); it("should not modify object schema", () => { linter.defineRule("mock-object-rule", mockObjectRule); - assert.deepStrictEqual(validator.getRuleOptionsSchema(linter.rules.get("mock-object-rule")), { + assert.deepStrictEqual(validator.getRuleOptionsSchema(ruleMapper("mock-object-rule")), { enum: ["first", "second"] }); }); @@ -418,43 +419,43 @@ describe("Validator", () => { describe("validateRuleOptions", () => { it("should throw for incorrect warning level number", () => { - const fn = validator.validateRuleOptions.bind(null, linter.rules.get("mock-rule"), "mock-rule", 3, "tests"); + const fn = validator.validateRuleOptions.bind(null, ruleMapper("mock-rule"), "mock-rule", 3, "tests"); assert.throws(fn, "tests:\n\tConfiguration for rule \"mock-rule\" is invalid:\n\tSeverity should be one of the following: 0 = off, 1 = warn, 2 = error (you passed '3').\n"); }); it("should throw for incorrect warning level string", () => { - const fn = validator.validateRuleOptions.bind(null, linter.rules.get("mock-rule"), "mock-rule", "booya", "tests"); + const fn = validator.validateRuleOptions.bind(null, ruleMapper("mock-rule"), "mock-rule", "booya", "tests"); assert.throws(fn, "tests:\n\tConfiguration for rule \"mock-rule\" is invalid:\n\tSeverity should be one of the following: 0 = off, 1 = warn, 2 = error (you passed '\"booya\"').\n"); }); it("should throw for invalid-type warning level", () => { - const fn = validator.validateRuleOptions.bind(null, linter.rules.get("mock-rule"), "mock-rule", [["error"]], "tests"); + const fn = validator.validateRuleOptions.bind(null, ruleMapper("mock-rule"), "mock-rule", [["error"]], "tests"); assert.throws(fn, "tests:\n\tConfiguration for rule \"mock-rule\" is invalid:\n\tSeverity should be one of the following: 0 = off, 1 = warn, 2 = error (you passed '[ \"error\" ]').\n"); }); it("should only check warning level for nonexistent rules", () => { - const fn = validator.validateRuleOptions.bind(null, linter.rules.get("non-existent-rule"), "non-existent-rule", [3, "foobar"], "tests"); + const fn = validator.validateRuleOptions.bind(null, ruleMapper("non-existent-rule"), "non-existent-rule", [3, "foobar"], "tests"); assert.throws(fn, "tests:\n\tConfiguration for rule \"non-existent-rule\" is invalid:\n\tSeverity should be one of the following: 0 = off, 1 = warn, 2 = error (you passed '3').\n"); }); it("should only check warning level for plugin rules", () => { - const fn = validator.validateRuleOptions.bind(null, linter.rules.get("plugin/rule"), "plugin/rule", 3, "tests"); + const fn = validator.validateRuleOptions.bind(null, ruleMapper("plugin/rule"), "plugin/rule", 3, "tests"); assert.throws(fn, "tests:\n\tConfiguration for rule \"plugin/rule\" is invalid:\n\tSeverity should be one of the following: 0 = off, 1 = warn, 2 = error (you passed '3').\n"); }); it("should throw for incorrect configuration values", () => { - const fn = validator.validateRuleOptions.bind(null, linter.rules.get("mock-rule"), "mock-rule", [2, "frist"], "tests"); + const fn = validator.validateRuleOptions.bind(null, ruleMapper("mock-rule"), "mock-rule", [2, "frist"], "tests"); assert.throws(fn, "tests:\n\tConfiguration for rule \"mock-rule\" is invalid:\n\tValue \"frist\" should be equal to one of the allowed values.\n"); }); it("should throw for too many configuration values", () => { - const fn = validator.validateRuleOptions.bind(null, linter.rules.get("mock-rule"), "mock-rule", [2, "first", "second"], "tests"); + const fn = validator.validateRuleOptions.bind(null, ruleMapper("mock-rule"), "mock-rule", [2, "first", "second"], "tests"); assert.throws(fn, "tests:\n\tConfiguration for rule \"mock-rule\" is invalid:\n\tValue [\"first\",\"second\"] should NOT have more than 1 items.\n"); }); diff --git a/tests/lib/config/plugins.js b/tests/lib/config/plugins.js index 37b96c0480f..afde8af4470 100644 --- a/tests/lib/config/plugins.js +++ b/tests/lib/config/plugins.js @@ -10,7 +10,6 @@ const assert = require("chai").assert, Plugins = require("../../../lib/config/plugins"), - Rules = require("../../../lib/rules"), Environments = require("../../../lib/config/environments"); const proxyquire = require("proxyquire").noCallThru().noPreserveCache(); @@ -24,7 +23,7 @@ describe("Plugins", () => { describe("get", () => { it("should return null when plugin doesn't exist", () => { - assert.isNull((new Plugins(new Environments(), new Rules())).get("foo")); + assert.isNull((new Plugins(new Environments(), () => {})).get("foo")); }); }); @@ -39,8 +38,8 @@ describe("Plugins", () => { beforeEach(() => { plugin = {}; scopedPlugin = {}; - rules = new Rules(); environments = new Environments(); + rules = new Map(); StubbedPlugins = new (proxyquire("../../../lib/config/plugins", { "eslint-plugin-example": plugin, "@scope/eslint-plugin-example": scopedPlugin, @@ -49,7 +48,7 @@ describe("Plugins", () => { throw new Error("error thrown while loading this module"); } } - }))(environments, rules); + }))(environments, rules.set.bind(rules)); }); it("should load a plugin when referenced by short name", () => { @@ -180,7 +179,7 @@ describe("Plugins", () => { }; StubbedPlugins.load("@scope/eslint-plugin-example"); - assert.isFalse(rules.getAllLoadedRules().has("example/foo")); + assert.isFalse(rules.has("example/foo")); }); }); }); @@ -189,17 +188,18 @@ describe("Plugins", () => { let StubbedPlugins, plugin1, - plugin2; - const rules = new Rules(), - environments = new Environments(); + plugin2, + rules; + const environments = new Environments(); beforeEach(() => { plugin1 = {}; plugin2 = {}; + rules = new Map(); StubbedPlugins = new (proxyquire("../../../lib/config/plugins", { "eslint-plugin-example1": plugin1, "eslint-plugin-example2": plugin2 - }))(environments, rules); + }))(environments, rules.set.bind(rules)); }); it("should load plugins when passed multiple plugins", () => { diff --git a/tests/lib/load-rules.js b/tests/lib/load-rules.js new file mode 100644 index 00000000000..9d01e6b0285 --- /dev/null +++ b/tests/lib/load-rules.js @@ -0,0 +1,25 @@ +/** + * @fileoverview Tests for loading rules from a directory + * @author Teddy Katz + */ + +"use strict"; + +const assert = require("chai").assert; +const loadRules = require("../../lib/load-rules"); + +describe("when given an invalid rules directory", () => { + it("should throw an error", () => { + assert.throws(() => { + loadRules("invalidDir"); + }); + }); +}); + +describe("when given a valid rules directory", () => { + it("should load rules and not throw an error", () => { + const rules = loadRules("tests/fixtures/rules", process.cwd()); + + assert.strictEqual(rules["fixture-rule"], require.resolve("../../tests/fixtures/rules/fixture-rule")); + }); +}); diff --git a/tests/lib/rules.js b/tests/lib/rules.js index 471600f654a..25abfd58224 100644 --- a/tests/lib/rules.js +++ b/tests/lib/rules.js @@ -24,25 +24,6 @@ describe("rules", () => { rules = new Rules(); }); - describe("when given an invalid rules directory", () => { - const code = "invaliddir"; - - it("should throw an error", () => { - assert.throws(() => { - rules.load(code); - }); - }); - }); - - describe("when given a valid rules directory", () => { - const code = "tests/fixtures/rules"; - - it("should load rules and not throw an error", () => { - rules.load(code, process.cwd()); - assert.strictEqual(typeof rules.get("fixture-rule"), "object"); - }); - }); - describe("when a rule has been defined", () => { it("should be able to retrieve the rule", () => { const ruleId = "michaelficarra"; @@ -108,30 +89,6 @@ describe("rules", () => { }); }); - describe("when importing plugin rules", () => { - const customPlugin = { - rules: { - "custom-rule"() { } - } - }, - pluginName = "custom-plugin"; - - it("should define all plugin rules with a qualified rule id", () => { - rules.importPlugin(customPlugin, pluginName); - - assert.isDefined(rules.get("custom-plugin/custom-rule")); - assert.strictEqual(rules.get("custom-plugin/custom-rule").create, customPlugin.rules["custom-rule"]); - }); - - it("should return custom rules as part of getAllLoadedRules", () => { - rules.importPlugin(customPlugin, pluginName); - - const allRules = rules.getAllLoadedRules(); - - assert.strictEqual(allRules.get("custom-plugin/custom-rule").create, customPlugin.rules["custom-rule"]); - }); - }); - describe("when loading all rules", () => { it("should return a map", () => { const allRules = rules.getAllLoadedRules();