Skip to content

Commit

Permalink
Chore: remove undocumented Linter#rules property (refs #9161) (#11335)
Browse files Browse the repository at this point in the history 
This removes the undocumented `rules` property from `Linter` instances, as part of the effort to remove undocumented API surface from `Linter` (also see #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.
  • Loading branch information
@not-an-aardvark
not-an-aardvark committed Jan 30, 2019
1 parent 36e3356 commit c1fd6f5
Show file tree
Hide file tree
Showing 10 changed files with 76 additions and 153 deletions.
40 changes: 5 additions & 35 deletions Makefile.js
View file
Expand Up @@ -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.
Expand Down Expand Up @@ -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);
};

Expand Down
5 changes: 3 additions & 2 deletions lib/cli-engine.js
View file
Expand Up @@ -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();
Expand Down Expand Up @@ -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));
});
}

Expand Down
2 changes: 1 addition & 1 deletion lib/config.js
View file
Expand Up @@ -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;
Expand Down
16 changes: 12 additions & 4 deletions lib/config/plugins.js
View file
Expand Up @@ -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;
}

/**
Expand All @@ -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);
});
}
}

/**
Expand Down
12 changes: 6 additions & 6 deletions lib/linter.js
View file
Expand Up @@ -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
Expand All @@ -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();
}

Expand Down Expand Up @@ -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
Expand All @@ -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,
Expand Down Expand Up @@ -957,7 +957,7 @@ module.exports = class Linter {
* @returns {void}
*/
defineRule(ruleId, ruleModule) {
this.rules.define(ruleId, ruleModule);
ruleMaps.get(this).define(ruleId, ruleModule);
}

/**
Expand All @@ -976,7 +976,7 @@ module.exports = class Linter {
* @returns {Map} All loaded rules
*/
getRules() {
return this.rules.getAllLoadedRules();
return ruleMaps.get(this).getAllLoadedRules();
}

/**
Expand Down
45 changes: 3 additions & 42 deletions lib/rules.js
View file
Expand Up @@ -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");

Expand Down Expand Up @@ -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]);
});
}

/**
Expand All @@ -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).
Expand Down
23 changes: 12 additions & 11 deletions tests/lib/config/config-validator.js
View file
Expand Up @@ -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();

//------------------------------------------------------------------------------
Expand Down Expand Up @@ -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(() => {
Expand Down Expand Up @@ -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"]
});
});
Expand All @@ -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");
});
Expand Down
18 changes: 9 additions & 9 deletions tests/lib/config/plugins.js
View file
Expand Up @@ -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();
Expand All @@ -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"));
});
});

Expand All @@ -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,
Expand All @@ -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", () => {
Expand Down Expand Up @@ -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"));
});
});
});
Expand All @@ -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", () => {
Expand Down

0 comments on commit c1fd6f5

Please sign in to comment.