New: no-lexical-globals rule

[mdjermanovic]

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

[X] New rule

Please describe what the rule should do:

Report global variables that are probably intended to be local to the script.

What category of rule is this? (place an "X" next to just one item)

[ ] Enforces code style
[ ] Warns about a potential error
[X] Suggests an alternate way of doing something
[ ] Other (please specify:)

Provide 2-3 code examples that this rule will warn about:

Applies to scripts only, not modules.

const a = 1;
let b;
class C {}

Why should this rule be included in ESLint (instead of a plugin)?

It's the best practice to avoid polluting the global scope.

I tried the following code with "eslint:all" and got no warnings:

const arry = [];
arry.reverse();

What changes did you make? (Give an overview)

New rule.

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

The warning message might be too long.

This rule might be merged in no-implicit-globals, but I guess that would be a breaking change.
These declarations are intentionally excluded from no-implicit-globals from the start (see #4565). I'm not sure what was the reason, they will not set properties on the global object but they still create global variables that could clash with variables from other scripts or shadow those directly set on the global object.

On the other hand, this might be a separate rule. I'm not sure how useful are const and let in the global scope, it might be still the best practice to use var foo = bar or window.foo = bar for globals. For example, there can be problems with TDZ:

<script>
    const SomeOptionalFeature = (function() {
      throw new Error(); // Something went wrong, this is an explicit throw just for example
    }());
</script>
<script>
  // This script might work without the optional feature, but will crash here.
  // Variable is declared but not initialized, this is TDZ although it's executed after the declaration.
  if (typeof SomeOptionalFeature !== "undefined") {
  }
</script>

I also think that both rules should be in "eslint:recommended", perhaps with a modification to support
/* exported */ like no-unused-vars.

[mysticatea]

Thank you for your contribute!

I'd like to update no-implicit-globals rule. How about the following steps?

1. (minor) Add lexicalBindings option to no-implicit-globals rule. Default is false. If it's set true, the no-implicit-globals rule warns let, const, and class declarations.
2. (major) Change the default of lexicalBindings option to true.

---

This script might work without the optional feature, but will crash here.

I'm very surprised by the fact. Because typeof foo doesn't throw any errors even if the foo was not defined. I suspect that it's a spec bug...

[mdjermanovic]

It might be also good to have an additional step between:

1. (minor) lexicalBindings = false
2. (minor) Support /* exported variableName */ (like the no-unused-vars rule)
3. (major) lexicalBindings = true

User would probably want to set lexicalBindings to true in the config, it prevents unintentional declarations in the global scope.

But, when the same user intends to declare one or two const, let or class variables in the global scope, there is no way to get around it other than with an /* eslint-disable */ comment. /* exported */ comments are nice and explicit.

It might be also useful for users who prefer var foo = bar over window.foo = bar, and these two are also not 100% same (e.g. var foo = bar sets configurable to false and cannot be shadowed by const, let and class, unlike window.foo = bar )

[mdjermanovic]

Also, I believe that the current message is not in line with the purpose of this rule, and that is to prevent global scope pollution (#4542).

E.g. if you have 10 variables, you'll see 10 messages like this:

Implicit global variable, assign as global property instead

That's more like encouraging the user to use a different way to pollute the global scope than preventing?

Perhaps the rule should suggest two options (like "wrap in IIFE if you want local, assign as global property if you want global"), though I'm not sure how short such message can be. Is there a convention for max message length?

[mysticatea]

It might be also good to have an additional step between: ...

Sounds good to me.

Perhaps the rule should suggest two options (like "wrap in IIFE if you want local, assign as global property if you want global"), though I'm not sure how short such message can be. Is there a convention for max message length?

Sounds good to me. There is no message length limitation currently.

[mdjermanovic]

I'm closing this because it's replaced with #11996