Skip to content
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

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

Merged
merged 7 commits into from Feb 1, 2019
@@ -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:

This comment has been minimized.

Copy link
@not-an-aardvark

not-an-aardvark Jan 31, 2019

Author Member

This documentation was previously misleading -- it implied that "writeable" was the default for global comments like /* global foo */, but in fact the default seems to be "readable".


```js
/* global var1:false, var2:false */
/* global var1:writeable, var2:writeable */
This conversation was marked as resolved by platinumazure

This comment has been minimized.

Copy link
@not-an-aardvark

not-an-aardvark Jan 31, 2019

Author Member

"writeable" and "writable" are both valid spellings. Although "writable" is apparently more common, I decided to use "writeable" for consistency with property from scope analysis exposed to rules, which is already called "writeable".

This comment has been minimized.

Copy link
@platinumazure

platinumazure Jan 31, 2019

Member

Should we tolerate both? (Or: For cases where the user uses the wrong spelling, do we error or default to writeable? I think I remember seeing a default to writeable elsewhere in this PR.)

This comment has been minimized.

Copy link
@not-an-aardvark

not-an-aardvark Jan 31, 2019

Author Member

We default to "writeable" for unknown values anyway, so technically "writable" is already supported as an alias (along with everything else). If we start reporting an error for unknown values in a major version I think it would be reasonable to keep tolerating "writable".

```

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:
This conversation was marked as resolved by aladdin-add

This comment has been minimized.

Copy link
@aladdin-add

aladdin-add Feb 2, 2019

Member

just noticed it was "readonly" in the proposal, is the renaming intentional?

This comment has been minimized.

Copy link
@not-an-aardvark

not-an-aardvark Feb 2, 2019

Author Member

Yes, this was intentional (readonly was also discussed in #9940 (comment)).

This comment has been minimized.

Copy link
@aladdin-add

aladdin-add Feb 2, 2019

Member

oh, missed it, thanks~~


```json
{
"globals": {
"var1": true,
"var2": false
"var1": "writeable",
"var2": "readable"
}
}
```
@@ -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
@@ -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";

This comment has been minimized.

Copy link
@not-an-aardvark

not-an-aardvark Jan 31, 2019

Author Member

Variable writability is communicated to rules via the variable.writeable boolean property in scope analysis.

Previously, a non-boolean value in a globals configuration would actually get passed directly to rules through scope analysis, so a configuration of {globals: {myGlobal: "foo"}} would end up as a variable object with variable.writeable = "foo".

Strictly speaking, we don't have any compatibility guarantees for non-boolean values here, since it's undocumented behavior. But since someone is probably relying on this by mistake (e.g. from a malformed config comment), I put in a fallback so that unexpected globals values default to "writeable" (and appear to rules as writeable: true). This fallback seems most likely to match existing behavior when users have malformed configs, since rules using this would typically be doing truthiness checks.

}
}
};
@@ -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 => {
@@ -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":
@@ -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 = {};

@@ -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;
}
@@ -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"]
This conversation was marked as resolved by platinumazure

This comment has been minimized.

Copy link
@platinumazure

platinumazure Jan 31, 2019

Member

Maybe worth adding ["writable", "writeable"] if we want to explicitly tolerate "writable" spelling?

].forEach(([input, output]) => {
it(util.inspect(input), () => {
assert.strictEqual(ConfigOps.normalizeConfigGlobal(input), output);
});
});
});
});
@@ -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 => {
@@ -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);
@@ -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 };
@@ -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", () => {
@@ -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
}
});
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.