Skip to content

Commit

Permalink
New: Allow globals to be disabled/configured with strings (fixes #9940)…
Browse files Browse the repository at this point in the history 
… (#11338)

* New: Allow globals to be disabled/configured with strings (fixes #9940)

As described in #9940, this allows globals in a config file to be specified with the strings `readable` and `writeable` rather than `false` and `true`, respectively. It also adds a new value, `off`, which turns off a previously-enabled global.

* Update lib/linter.js

Co-Authored-By: not-an-aardvark <teddy.katz@gmail.com>

* Update docs/user-guide/configuring.md

Co-Authored-By: not-an-aardvark <teddy.katz@gmail.com>

* Add test for "writable"

* Remove unnecessary map/destructuring

* Adjust confusing variable name

* Move note about legacy booleans down
  • Loading branch information
@not-an-aardvark @btmills
not-an-aardvark authored and btmills committed Feb 1, 2019
1 parent dccee63 commit 0a3c3ff
Show file tree
Hide file tree
Showing 7 changed files with 164 additions and 67 deletions.
29 changes: 22 additions & 7 deletions docs/user-guide/configuring.md
View file
Expand Up @@ -208,19 +208,19 @@ To specify globals using a comment inside of your JavaScript file, use the follo
/* global var1, var2 */
```

This defines two global variables, `var1` and `var2`. If you want to optionally specify that these global variables should never be written to (only read), then you can set each with a `false` flag:
This defines two global variables, `var1` and `var2`. If you want to optionally specify that these global variables can be written to (rather than only being read), then you can set each with a `writeable` flag:

```js
/* global var1:false, var2:false */
/* global var1:writeable, var2:writeable */
```

To configure global variables inside of a configuration file, use the `globals` key and indicate the global variables you want to use. Set each global variable name equal to `true` to allow the variable to be overwritten or `false` to disallow overwriting. For example:
To configure global variables inside of a configuration file, set the `globals` configuration property to an object containing keys named for each of the global variables you want to use. For each global variable key, set the corresponding value equal to `"writeable"` to allow the variable to be overwritten or `"readable"` to disallow overwriting. For example:

```json
{
"globals": {
"var1": true,
"var2": false
"var1": "writeable",
"var2": "readable"
}
}
```
Expand All @@ -230,12 +230,27 @@ And in YAML:
```yaml
---
globals:
var1: true
var2: false
var1: writeable
var2: readable
```

These examples allow `var1` to be overwritten in your code, but disallow it for `var2`.

Globals can be disabled with the string `"off"`. For example, in an environment where most ES2015 globals are available but `Promise` is unavailable, you might use this config:

```json
{
"env": {
"es6": true
},
"globals": {
"Promise": "off"
}
}
```

For historical reasons, the boolean values `false` and `true` can also be used to configure globals, and are equivalent to `"readable"` and `"writeable"`, respectively. However, this usage of booleans is deprecated.

**Note:** Enable the [no-global-assign](../rules/no-global-assign.md) rule to disallow modifications to read-only global variables in your code.

## Configuring Plugins
Expand Down
28 changes: 28 additions & 0 deletions lib/config/config-ops.js
View file
Expand Up @@ -370,5 +370,33 @@ module.exports = {

return patternList.some(pattern => minimatch(filePath, pattern, opts)) &&
!excludedPatternList.some(excludedPattern => minimatch(filePath, excludedPattern, opts));
},

/**
* Normalizes a value for a global in a config
* @param {(boolean|string|null)} configuredValue The value given for a global in configuration or in
* a global directive comment
* @returns {("readable"|"writeable"|"off")} The value normalized as a string
*/
normalizeConfigGlobal(configuredValue) {
switch (configuredValue) {
case "off":
return "off";

case true:
case "true":
case "writeable":
return "writeable";

case null:
case false:
case "false":
case "readable":
return "readable";

// Fallback to minimize compatibility impact
default:
return "writeable";
}
}
};
50 changes: 25 additions & 25 deletions lib/linter.js
View file
Expand Up @@ -70,30 +70,30 @@ const commentParser = new ConfigCommentParser();
* @returns {void}
*/
function addDeclaredGlobals(globalScope, configGlobals, commentDirectives) {
Object.keys(configGlobals).forEach(name => {
let variable = globalScope.set.get(name);

if (!variable) {
variable = new eslintScope.Variable(name, globalScope);
variable.eslintExplicitGlobal = false;
globalScope.variables.push(variable);
globalScope.set.set(name, variable);
}
variable.writeable = configGlobals[name];
});

Object.keys(commentDirectives.enabledGlobals).forEach(name => {
let variable = globalScope.set.get(name);
const mergedGlobalsInfo = Object.assign(
{},
lodash.mapValues(configGlobals, value => ({ sourceComment: null, value: ConfigOps.normalizeConfigGlobal(value) })),
lodash.mapValues(commentDirectives.enabledGlobals, ({ comment, value }) => ({ sourceComment: comment, value: ConfigOps.normalizeConfigGlobal(value) }))
);

if (!variable) {
variable = new eslintScope.Variable(name, globalScope);
variable.eslintExplicitGlobal = true;
variable.eslintExplicitGlobalComment = commentDirectives.enabledGlobals[name].comment;
globalScope.variables.push(variable);
globalScope.set.set(name, variable);
}
variable.writeable = commentDirectives.enabledGlobals[name].value;
});
Object.keys(mergedGlobalsInfo)
.filter(name => mergedGlobalsInfo[name].value !== "off")
.forEach(name => {
let variable = globalScope.set.get(name);

if (!variable) {
variable = new eslintScope.Variable(name, globalScope);
if (mergedGlobalsInfo[name].sourceComment === null) {
variable.eslintExplicitGlobal = false;
} else {
variable.eslintExplicitGlobal = true;
variable.eslintExplicitGlobalComment = mergedGlobalsInfo[name].sourceComment;
}
globalScope.variables.push(variable);
globalScope.set.set(name, variable);
}
variable.writeable = (mergedGlobalsInfo[name].value === "writeable");
});

// mark all exported variables as such
Object.keys(commentDirectives.exportedVariables).forEach(name => {
Expand Down Expand Up @@ -191,12 +191,12 @@ function getDirectiveComments(filename, ast, ruleMapper) {
} else if (comment.type === "Block") {
switch (match[1]) {
case "exported":
Object.assign(exportedVariables, commentParser.parseBooleanConfig(directiveValue, comment));
Object.assign(exportedVariables, commentParser.parseStringConfig(directiveValue, comment));
break;

case "globals":
case "global":
Object.assign(enabledGlobals, commentParser.parseBooleanConfig(directiveValue, comment));
Object.assign(enabledGlobals, commentParser.parseStringConfig(directiveValue, comment));
break;

case "eslint-disable":
Expand Down
17 changes: 7 additions & 10 deletions lib/util/config-comment-parser.js
View file
Expand Up @@ -26,14 +26,14 @@ const debug = require("debug")("eslint:config-comment-parser");
module.exports = class ConfigCommentParser {

/**
* Parses a list of "name:boolean_value" or/and "name" options divided by comma or
* Parses a list of "name:string_value" or/and "name" options divided by comma or
* whitespace. Used for "global" and "exported" comments.
* @param {string} string The string to parse.
* @param {Comment} comment The comment node which has the string.
* @returns {Object} Result map object of names and boolean values
* @returns {Object} Result map object of names and string values, or null values if no value was provided
*/
parseBooleanConfig(string, comment) {
debug("Parsing Boolean config");
parseStringConfig(string, comment) {
debug("Parsing String config");

const items = {};

Expand All @@ -45,13 +45,10 @@ module.exports = class ConfigCommentParser {
return;
}

// value defaults to "false" (if not provided), e.g: "foo" => ["foo", "false"]
const [key, value = "false"] = name.split(":");
// value defaults to null (if not provided), e.g: "foo" => ["foo", null]
const [key, value = null] = name.split(":");

items[key] = {
value: value === "true",
comment
};
items[key] = { value, comment };
});
return items;
}
Expand Down
19 changes: 19 additions & 0 deletions tests/lib/config/config-ops.js
View file
Expand Up @@ -898,4 +898,23 @@ describe("ConfigOps", () => {
error("foo.js", ["/foo.js"], "Invalid override pattern (expected relative path not containing '..'): /foo.js");
error("foo.js", ["../**"], "Invalid override pattern (expected relative path not containing '..'): ../**");
});

describe("normalizeConfigGlobal", () => {
[
["off", "off"],
[true, "writeable"],
["true", "writeable"],
[false, "readable"],
["false", "readable"],
[null, "readable"],
["writeable", "writeable"],
["readable", "readable"],
["writable", "writeable"],
["something else", "writeable"]
].forEach(([input, output]) => {
it(util.inspect(input), () => {
assert.strictEqual(ConfigOps.normalizeConfigGlobal(input), output);
});
});
});
});
46 changes: 42 additions & 4 deletions tests/lib/linter.js
View file
Expand Up @@ -1124,10 +1124,15 @@ describe("Linter", () => {
});

describe("when evaluating code containing /*global */ and /*globals */ blocks", () => {
const code = "/*global a b:true c:false*/ function foo() {} /*globals d:true*/";

it("variables should be available in global scope", () => {
const config = { rules: { checker: "error" } };
const config = { rules: { checker: "error" }, globals: { Array: "off", ConfigGlobal: "writeable" } };
const code = `
/*global a b:true c:false d:readable e:writeable Math:off */
function foo() {}
/*globals f:true*/
/* global ConfigGlobal : readable */
`;
let spy;

linter.defineRule("checker", context => {
Expand All @@ -1136,7 +1141,12 @@ describe("Linter", () => {
const a = getVariable(scope, "a"),
b = getVariable(scope, "b"),
c = getVariable(scope, "c"),
d = getVariable(scope, "d");
d = getVariable(scope, "d"),
e = getVariable(scope, "e"),
f = getVariable(scope, "f"),
mathGlobal = getVariable(scope, "Math"),
arrayGlobal = getVariable(scope, "Array"),
configGlobal = getVariable(scope, "ConfigGlobal");

assert.strictEqual(a.name, "a");
assert.strictEqual(a.writeable, false);
Expand All @@ -1145,7 +1155,15 @@ describe("Linter", () => {
assert.strictEqual(c.name, "c");
assert.strictEqual(c.writeable, false);
assert.strictEqual(d.name, "d");
assert.strictEqual(d.writeable, true);
assert.strictEqual(d.writeable, false);
assert.strictEqual(e.name, "e");
assert.strictEqual(e.writeable, true);
assert.strictEqual(f.name, "f");
assert.strictEqual(f.writeable, true);
assert.strictEqual(mathGlobal, null);
assert.strictEqual(arrayGlobal, null);
assert.strictEqual(configGlobal.name, "ConfigGlobal");
assert.strictEqual(configGlobal.writeable, false);
});

return { Program: spy };
Expand Down Expand Up @@ -1454,6 +1472,26 @@ describe("Linter", () => {
linter.verify(code, config);
assert(spy && spy.calledOnce);
});

it("ES6 global variables can be disabled when the es6 environment is enabled", () => {
const config = { rules: { checker: "error" }, globals: { Promise: "off", Symbol: "off", WeakMap: "off" }, env: { es6: true } };
let spy;

linter.defineRule("checker", context => {
spy = sandbox.spy(() => {
const scope = context.getScope();

assert.strictEqual(getVariable(scope, "Promise"), null);
assert.strictEqual(getVariable(scope, "Symbol"), null);
assert.strictEqual(getVariable(scope, "WeakMap"), null);
});

return { Program: spy };
});

linter.verify(code, config);
assert(spy && spy.calledOnce);
});
});

describe("at any time", () => {
Expand Down
42 changes: 21 additions & 21 deletions tests/lib/util/config-comment-parser.js
View file
Expand Up @@ -117,77 +117,77 @@ describe("ConfigCommentParser", () => {

});

describe("parseBooleanConfig", () => {
describe("parseStringConfig", () => {

const comment = {};

it("should parse Boolean config with one item", () => {
it("should parse String config with one item", () => {
const code = "a: true";
const result = commentParser.parseBooleanConfig(code, comment);
const result = commentParser.parseStringConfig(code, comment);

assert.deepStrictEqual(result, {
a: {
value: true,
value: "true",
comment
}
});
});

it("should parse Boolean config with one item and no value", () => {
it("should parse String config with one item and no value", () => {
const code = "a";
const result = commentParser.parseBooleanConfig(code, comment);
const result = commentParser.parseStringConfig(code, comment);

assert.deepStrictEqual(result, {
a: {
value: false,
value: null,
comment
}
});
});

it("should parse Boolean config with two items", () => {
const code = "a: true b:false";
const result = commentParser.parseBooleanConfig(code, comment);
it("should parse String config with two items", () => {
const code = "a: five b:three";
const result = commentParser.parseStringConfig(code, comment);

assert.deepStrictEqual(result, {
a: {
value: true,
value: "five",
comment
},
b: {
value: false,
value: "three",
comment
}
});
});

it("should parse Boolean config with two comma-separated items", () => {
const code = "a: true, b:false";
const result = commentParser.parseBooleanConfig(code, comment);
it("should parse String config with two comma-separated items", () => {
const code = "a: seventy, b:ELEVENTEEN";
const result = commentParser.parseStringConfig(code, comment);

assert.deepStrictEqual(result, {
a: {
value: true,
value: "seventy",
comment
},
b: {
value: false,
value: "ELEVENTEEN",
comment
}
});
});

it("should parse Boolean config with two comma-separated items and no values", () => {
it("should parse String config with two comma-separated items and no values", () => {
const code = "a , b";
const result = commentParser.parseBooleanConfig(code, comment);
const result = commentParser.parseStringConfig(code, comment);

assert.deepStrictEqual(result, {
a: {
value: false,
value: null,
comment
},
b: {
value: false,
value: null,
comment
}
});
Expand Down

0 comments on commit 0a3c3ff

Please sign in to comment.