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

Update: Add 'lexicalBindings' to no-implicit-globals and change messages #11996

Merged
merged 1 commit into from Nov 22, 2019

Conversation

@mdjermanovic
Copy link
Member

mdjermanovic commented Jul 16, 2019

What is the purpose of this pull request? (put an "X" next to item)

[X] Changes an existing rule

Two changes based on #11981


-- New option --

What rule do you want to change?

no-implicit-globals

Does this change cause the rule to produce more or fewer warnings?

Same by default, more if the option is set.

How will the change be implemented? (New option, new default behavior, etc.)?

New option.

Please provide some example code that this change will affect:

/*eslint no-implicit-globals: "error"*/

const a = 1;

let b;

class C {}

What does the rule currently do for this code?

No warnings.

What will the rule do after it's changed?

/*eslint no-implicit-globals: ["error", {"lexicalBindings": true}]*/

// Unexpected 'const' declaration in the global scope, wrap in a block or in an IIFE.
const a = 1; 

// Unexpected 'let' declaration in the global scope, wrap in a block or in an IIFE.
let b;

// Unexpected class declaration in the global scope, wrap in a block or in an IIFE.
class C {}

-- Changed messages --

What rule do you want to change?

no-implicit-globals

Does this change cause the rule to produce more or fewer warnings?

Same.

How will the change be implemented? (New option, new default behavior, etc.)?

Same behavior with new messages.

Please provide some example code that this change will affect:

/*eslint no-implicit-globals: "error"*/

function f() {}

var a;

b = 1;

var Array;

Object = {};

What does the rule currently do for this code?

/*eslint no-implicit-globals: "error"*/

// Implicit global variable, assign as global property instead.
function f() {}

// Implicit global variable, assign as global property instead.
var a;

// Implicit global variable, assign as global property instead.
b = 1;

// Implicit global variable, assign as global property instead.
var Array;

// Implicit global variable, assign as global property instead.
Object = {};

What will the rule do after it's changed?

/*eslint no-implicit-globals: "error"*/

// Unexpected function declaration in the global scope, wrap in an IIFE for a local variable, assign as global property for a global variable.
function f() {}

// Unexpected 'var' declaration in the global scope, wrap in an IIFE for a local variable, assign as global property for a global variable.
var a;

// Global variable leak, declare the variable if it is intended to be local.
b = 1;

// Unexpected redeclaration of read-only global variable.
var Array;

// Unexpected assignment to read-only global variable.
Object = {};

What changes did you make? (Give an overview)

Added the new option and changed messages.

Is there anything you'd like reviewers to focus on?

Some messages might be too long.

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Sep 28, 2019

Sorry for losing track of this!

Question: What should happen in module scope, or in the CJS scope? I assume we would want no messages in those cases for lexical declarations as they wouldn't be leaking to global scope?

@kaicataldo

This comment has been minimized.

Copy link
Member

kaicataldo commented Sep 28, 2019

I think it makes sense to make the rule aware of whether sourceType is set to "script" or "module".

@ilyavolodin ilyavolodin added accepted and removed evaluating labels Sep 28, 2019
@mdjermanovic

This comment has been minimized.

Copy link
Member Author

mdjermanovic commented Sep 28, 2019

That should already work well, as the code uses scope manager to find declared globals only (doesn't just report all top-level declarations).

These are some test cases from valid[]:

        // Different scoping
        {
            code: "var foo = 1;",
            parserOptions: { ecmaVersion: 2015, sourceType: "module" }
        },
        {
            code: "function foo() {}",
            parserOptions: { ecmaVersion: 2015, sourceType: "module" }
        },
        {
            code: "function *foo() {}",
            parserOptions: { ecmaVersion: 2015, sourceType: "module" }
        },
        {
            code: "var foo = 1;",
            parserOptions: { ecmaFeatures: { globalReturn: true } }
        },
        {
            code: "function foo() {}",
            parserOptions: { ecmaFeatures: { globalReturn: true } }
        },
        {
            code: "var foo = 1;",
            env: { node: true }
        },
        {
            code: "function foo() {}",
            env: { node: true }
        },
        // different scoping
        {
            code: "const foo = 1; let bar; class Baz {}",
            parserOptions: { ecmaVersion: 2015, sourceType: "module" }
        },
        {
            code: "const foo = 1; let bar; class Baz {}",
            parserOptions: { ecmaVersion: 2015 },
            env: { node: true }
        },
        {
            code: "const foo = 1; let bar; class Baz {}",
            parserOptions: { ecmaVersion: 2015, ecmaFeatures: { globalReturn: true } }
        },
@mdjermanovic

This comment has been minimized.

Copy link
Member Author

mdjermanovic commented Sep 28, 2019

It might be also good to revisit this rule at some point. Seems that the rule overlaps a lot with other rules, namely no-global-assign, no-redeclare and no-undef.

In particular, I'm not sure why this rule disallows assignments to readonly globals. Also, why the rule allows global var declaration for writable globals because the preferred way is to assign to the global object instead.

(this PR didn't change behavior, the messages are just more explicit about the reason for an error).

Copy link
Member

kaicataldo left a comment

Thanks for the all tests and the fantastic documentation! LGTM.

Copy link
Member

btmills left a comment

This is a phenomenal improvement to the documentation! I really like what you've done with the messages. They should be much more helpful to users. Implementation LGTM, tests look plenty thorough. Nice work @mdjermanovic!

@btmills btmills merged commit 5c68f5f into eslint:master Nov 22, 2019
9 checks passed
9 checks passed
commit-message Commit message follows guidelines
Details
continuous-integration Build #20190716.1 succeeded
Details
continuous-integration (Test on Node.js 10 (Linux)) Test on Node.js 10 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Linux)) Test on Node.js 12 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Windows)) Test on Node.js 12 (Windows) succeeded
Details
continuous-integration (Test on Node.js 12 (macOS)) Test on Node.js 12 (macOS) succeeded
Details
continuous-integration (Test on Node.js 8 (Linux)) Test on Node.js 8 (Linux) succeeded
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.