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

Rule: prevent global scope var/function pollution #4542

Closed
josh opened this Issue Nov 25, 2015 · 25 comments

Comments

Projects
None yet
6 participants
@josh
Copy link
Contributor

josh commented Nov 25, 2015

I feel a little dumb asking this, but is there already a rule for preventing var and function global scope leaks in browsers?

The following patterns are considered problems:

function foo() {
}
var bar = 1;

The following patterns are considered not problems:

// using explicit window
window.foo = function() {
};
window.bar = 1;
// using iife
(function() {
  function foo() {
  }
  var bar = 1;
})();

Thanks!

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Nov 25, 2015

@josh Thanks for the issue! If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

@eslintbot eslintbot added the triage label Nov 25, 2015

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 25, 2015

What do you mean by "global scope leaks"? Do you mean you want a warning whenever a global variable is defined?

@josh

This comment has been minimized.

Copy link
Contributor Author

josh commented Nov 25, 2015

Do you mean you want a warning whenever a global variable is defined?

Yeah, without an explicit window or self assignment.

Its pretty common for developers to leak global helper functions if they are working outside an IIFE scope.

function onClick(el) {
}

$(".foo").on("click", onClick)

Suggest using an IIFE if they didn't mean for onClick to be global. And if they did, they should explicitly assign it as a window property.

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Nov 25, 2015

@nzakas Only through top-level declarations in a script.

@josh

This comment has been minimized.

Copy link
Contributor Author

josh commented Nov 25, 2015

Only through top-level declarations in a script

Yep.

@josh

This comment has been minimized.

Copy link
Contributor Author

josh commented Nov 25, 2015

@michaelficarra still used to the CoffeeScript world where a file based IIFE is implied 😄

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Nov 25, 2015

@josh Is GitHub finally dumping CoffeeScript?

@josh

This comment has been minimized.

Copy link
Contributor Author

josh commented Nov 25, 2015

@michaelficarra haha, yeah. Thats the plan. Basically getting the new env setup with babel and eslint. @atom is making the same transition as well.

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Nov 25, 2015

Sorry, I might be missing some context here. For browser script, what exactly is a difference between window.foo = function() {...} and function foo() {...} and why would you want to enforce first one? I would think that in most cases you would want the second one, since the script might be wrapped in the IIFE and in that case foo is not going to be polluting global scope.

@josh

This comment has been minimized.

Copy link
Contributor Author

josh commented Nov 25, 2015

For browser script, what exactly is a difference between window.foo = function() {...} and function foo() {...} and why would you want to enforce first one?

window.foo is explicitly setting a global. Most people don't realize function foo() {...} will be made a global if its defined at the root scope.

since the script might be wrapped in the IIFE and in that case foo is not going to be polluting global scope.

You statically check to ensure thats the case.

// using iife, its okay
(function() {
  function foo() {
  }
  var bar = 1;
})();
@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Nov 25, 2015

Ahh.. Ok, so this is a stylistic rule request, to lower possible confusion. OK, got it.

@josh

This comment has been minimized.

Copy link
Contributor Author

josh commented Nov 25, 2015

Ahh.. Ok, so this is a stylistic rule request, to lower possible confusion. OK, got it.

Definitely.

@josh

This comment has been minimized.

Copy link
Contributor Author

josh commented Nov 25, 2015

Theres also some interesting ES6 block scoping cases,

{
  // bad
  var a

  // ok, scoped to block, won't set a global
  let b
  const c
  function d() {}
}

// still bad
var x
let y
const z
@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Nov 25, 2015

@ilyavolodin The goal is to force developers to explicitly distinguish between the two forms so you don't ever do one while expecting the behaviour of the other.

@josh It's about time. FYI, in the future, you can just use modules and then top-level declarations don't implicitly create properties on the global object.

@josh

This comment has been minimized.

Copy link
Contributor Author

josh commented Nov 25, 2015

FYI, in the future, you can just use modules and then top-level declarations don't implicitly create properties on the global object.

@michaelficarra yeah, I'm planning on keeping "scripts" and "modules" organized separately. Then only apply this rule in the "scripts" .eslintrc. None of our current code uses modules yet, but we'll make that transition in another step.

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Nov 25, 2015

@josh Actually, you said you were using babel. In that case, you should be using modules right now and this won't be a problem. Unless you need sloppy mode for something? But I doubt that.

edit: Repeatedly crossing streams with the above comments, sorry.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 27, 2015

Seems like a good idea. @josh do you want to submit a PR? We are pretty shorthanded at the moment, so otherwise this may not get done.

@josh

This comment has been minimized.

Copy link
Contributor Author

josh commented Nov 27, 2015

@josh do you want to submit a PR?

Yeah, sure, I'll give it a shot!

@josh

This comment has been minimized.

Copy link
Contributor Author

josh commented Nov 27, 2015

Any suggestions on the rule name? Seems like it should fall under the current "Variables" category. no-global-vars, no-global-scope-vars, no-top-level-scope-vars?

@mgol

This comment has been minimized.

Copy link
Contributor

mgol commented Nov 27, 2015

@josh no-implicit-globals?

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 27, 2015

👍 no-implicit-globals

There are edge cases to be aware of when ecmaFeatures.globalReturn or ecmaFeatures.modules are true.

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Nov 27, 2015

What is the globalReturn edge case?

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 27, 2015

Same as modules - the topmost scope is not global.

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Nov 27, 2015

I didn't know that that was implied by the globalReturn flag. I thought it just permitted top level returns in the parser.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 28, 2015

It's also used as a trigger to inject an extra scope to mimic Node.js scoping.

@nzakas nzakas closed this in 48ee999 Dec 11, 2015

nzakas added a commit that referenced this issue Dec 11, 2015

Merge pull request #4565 from josh/issue4542
New: Add no-implicit-globals rule (fixes #4542)

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.