eslint: Don't add event as global when browser: true

Right now when you configure your env with “browser: true” you will get a event variable automatically added to your globals so this would lint perfectly despite event not being defined (tested in the online demo):

/*eslint-env browser */

function a() {
  var b = event.target;
  return b;
}

a();

I think that this is incorrect as the event variable is only defined inside an event context, so its not totally global and may end up preventing eslint form catching some potential undefined variables.

Also, and the actual reason why I discovered this, in some contexts the event parameter and the event global may not be the same thing. For example in React they are using some fake nodes to dispatch events (I find it a bit odd but this or another similar code may still be a valid use case) and they do a bind to override the event value as seen here: https://github.com/facebook/react/blob/8c75e792abf54f34b7a36a777826f104d1e0fb34/src/shared/utils/ReactErrorUtils.js#L68 . In that case if you use the global event instead of the parameter you will get a fake <react></react> node instead of the target and you won’t figure out until one hour later 😄

For that cases if you don’t think that removing the global event for the browsers is a good idea, potentially for compatibility reasons, maybe adding an specific rule to forbid the use of global event variables would be a good idea so eslint can handle this situations a bit better.

About this issue

Original URL
State: closed
Created 9 years ago
Reactions: 1
Comments: 27 (10 by maintainers)

Most upvoted comments

Just throwing in my 2c here. Global event fails in both modern versions of IE and Firefox (but passes in Chrome). Since I do all my development in chrome this one just slipped by ESLint and (thankfully) ended up being caught in a PR review:

  onMouseDown() { // <-- missing `event` argument
    if (event.button === 0 && event.target === this.$element.get(0)) {
      this.startTranslate(event);
    }
  }

I’d certainly prefer a linter error if an undefined event exists in scope. My take is that if you’re doing this deliberately, then you can add manually the event global to your config (or just reference it explicitly as window.event). Alternatively offer some additional configuration option like browser-legacy or similar?

Seems silly to allow code that will break in modern browsers in preference for permitting code that is only required by old browsers.

rhys-vdw on May 24, 2016

Note, you can use the no-restricted-globals rule now: http://eslint.org/docs/rules/no-restricted-globals

axelson on Aug 30, 2016

@rhys-vdw Well as the one I opened the ticket originally I totally agree 😄 … but its kind of fixed now with the implementation of “no-restricted-globals” which basically allow you to prevent the use of any globals. So if you do:

no-restricted-globals: [2, "event"]

Then in theory (to be honest I didn’t test it yet) any uses of event as global will show an error in eslint. So basically a new default setting in all my eslint config files 😛

adrian-seijo on May 24, 2016

Thanks for the good discussion. We aren’t going to remove event at this time.

nzakas on Oct 8, 2015