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
Copy path View file
@@ -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, use the `globals` key and indicate the global variables you want to use. Set each global variable name equal to `writeable` to allow the variable to be overwritten or `readable` to disallow overwriting. For example:
This conversation was marked as resolved by not-an-aardvark

This comment has been minimized.

Copy link
@platinumazure

platinumazure Jan 31, 2019

Member

I'm not a fan of the current syntax and word choices in this paragraph (which were issues before this change). I took a stab at improving it, but it could probably be improved further.

What you think of this as a starting point?

Suggested change
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 `writeable` to allow the variable to be overwritten or `readable` 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 comment has been minimized.

Copy link
@not-an-aardvark

not-an-aardvark Jan 31, 2019

Author Member

That rewording sounds good to me -- I can't think of any additional improvements.


```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`.

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.

This comment has been minimized.

Copy link
@platinumazure

platinumazure Jan 31, 2019

Member

Not a blocker: Do we have a way to stylistically indicate that this is an aside? (I'm assuming not and that this would be a wider information architecture question, but wanted to ask.)

Also: I wonder if this should go below the next section on disabling globals? (Edit: Or another possibility might be to use subsections for enabling globals [read/write], and disabling [off], in which case the paragraph probably shouldn't move, but everything would flow a little better because the subsection headers would serve as natural breaks.)

This comment has been minimized.

Copy link
@not-an-aardvark

not-an-aardvark Jan 31, 2019

Author Member

I moved this line below the section about disabling globals.

I think this line will be useful for users figuring out what's going on in existing configs containing booleans, so I don't want to make it too much of a footnote. I was thinking about preceding this line with "Note:", but in the new location this line is followed by another line that also starts with "Note:", which would look a bit strange. That said, I'm open to stylistic suggestions.


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"
}
}
```

**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
Copy path View file
@@ -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.

}
}
};
Copy path View file
@@ -70,30 +70,31 @@ 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 allGlobals = Object.assign(
This conversation was marked as resolved by not-an-aardvark

This comment has been minimized.

Copy link
@platinumazure

platinumazure Jan 31, 2019

Member

Nit: I don't particularly like this variable name, as this is not just globals, but rather an object with globals as keys and an object with comment information and global config as a value. If nothing else, this might deserve a JSDoc type to clarify the expected object structure. I'll leave it to you to identify the best path forward here.

This comment has been minimized.

Copy link
@not-an-aardvark

not-an-aardvark Jan 31, 2019

Author Member

Updated the variable name to hopefully prevent it from being misleading -- let me know if that's better.

{},
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(allGlobals)
.map(name => [name, allGlobals[name]])
This conversation was marked as resolved by not-an-aardvark

This comment has been minimized.

Copy link
@platinumazure

platinumazure Jan 31, 2019

Member

Nit: This feels like a wasteful map for the sole purpose of allowing nifty destructures later. Couldn't we just access the object in the filter predicate and map projection?

E.g.:

Object.keys(allGlobals)
    .filter(key => allGlobals[key].value !== "off")
    .forEach(name => {
        const { sourceComment, value } = allGlobals[name];
        // etc.
    });

This comment has been minimized.

Copy link
@not-an-aardvark

not-an-aardvark Jan 31, 2019

Author Member

Updated.

.filter(([, { value }]) => value !== "off")
.forEach(([name, { sourceComment, value }]) => {
let variable = globalScope.set.get(name);

if (!variable) {
variable = new eslintScope.Variable(name, globalScope);
if (sourceComment === null) {
variable.eslintExplicitGlobal = false;
} else {
variable.eslintExplicitGlobal = true;
variable.eslintExplicitGlobalComment = sourceComment;
}
globalScope.variables.push(variable);
globalScope.set.set(name, variable);
}
variable.writeable = value === "writeable";
This conversation was marked as resolved by not-an-aardvark

This comment has been minimized.

Copy link
@platinumazure

platinumazure Jan 31, 2019

Member

Nit: Although our lint config doesn't enforce this and although it isn't explicitly stated, the coding conventions documentation does seem to favor parenthesizing equality checks assigned directly to a variable even though they are not syntactically required.

Suggested change
variable.writeable = value === "writeable";
variable.writeable = (value === "writeable");
});

// mark all exported variables as such
Object.keys(commentDirectives.exportedVariables).forEach(name => {
@@ -191,12 +192,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;
}
Copy path View file
@@ -898,4 +898,22 @@ 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"],
["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);
});
});
});
});
Copy path View file
@@ -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.