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

Add a no-restricted-globals rule to prevent the use of the specific globals [$25] #3966

Closed
kileras opened this Issue Sep 28, 2015 · 21 comments

Comments

Projects
None yet
10 participants
@kileras

kileras commented Sep 28, 2015

So following my comments in #3964, I think that adding a rule to prevent the use of the global event variable will be useful and potentially avoid some issues like the one described with react due to the magic of the global event variable.

There is a $25 open bounty on this issue. Add to the bounty at Bountysource.

@eslintbot eslintbot added the triage label Sep 28, 2015

@eslintbot

This comment has been minimized.

Show comment
Hide comment
@eslintbot

eslintbot Sep 28, 2015

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 commented Sep 28, 2015

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.

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Sep 28, 2015

Member

There's probably a number of historic globals that can be avoided entirely.

Member

michaelficarra commented Sep 28, 2015

There's probably a number of historic globals that can be avoided entirely.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Sep 29, 2015

Member

I don't see us adding a rule just to block use of one variable. #3358 is somewhat related.

Maybe we could have a rule that complains on the use of a user-specified list of variables.

@kileras can you explain the use case you're thinking of? Also, note the link to our rule proposal guidelines in the first comment

Member

nzakas commented Sep 29, 2015

I don't see us adding a rule just to block use of one variable. #3358 is somewhat related.

Maybe we could have a rule that complains on the use of a user-specified list of variables.

@kileras can you explain the use case you're thinking of? Also, note the link to our rule proposal guidelines in the first comment

@nzakas nzakas added rule feature evaluating and removed triage labels Sep 29, 2015

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Sep 29, 2015

Member

@nzakas It's different from #3358 because it's not a "bad identifier"; you just don't want to reference a global of that name. Shadowing that global should be fine because you'd like to pretend it isn't even there.

Member

michaelficarra commented Sep 29, 2015

@nzakas It's different from #3358 because it's not a "bad identifier"; you just don't want to reference a global of that name. Shadowing that global should be fine because you'd like to pretend it isn't even there.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Sep 29, 2015

Member

I know it's different, hence the word "somewhat" :)

I'm trying to cross reference rules dealing with identifiers because there are a lot.

Member

nzakas commented Sep 29, 2015

I know it's different, hence the word "somewhat" :)

I'm trying to cross reference rules dealing with identifiers because there are a lot.

@kileras

This comment has been minimized.

Show comment
Hide comment
@kileras

kileras Sep 30, 2015

Well the global event variable is a magic variable that was created a really long time ago as the way of inline event listeners to have access to the event object like:

<body onclick="console.log(event);">

Nowadays thats not used anymore, or shouldn't at least, so the use of that globally defined event variable should be avoided and it's a bad practice and the event listener receive an event object. Also In some situations, as I mention in the other issue, that's even worse as some libraries that modify for whatever reason the event that the listener receives, or create a new one, so it won't necessarily match the global one.

So basically the event is a very specific kind of global variable, created to support some functionality that should not be used at all, magically populated when an event is fired and its used is considered a bad practice, thats why I think it may deserve a specific rule.

But anyway it turns out that while playing with it I discovered that actually event is not really global in all the browsers, it is only in webkit/blink and trident, in Firefox (and potentially old opera didn't test) the event global variable is only defined in inline listener like the one above, the rest of the time it does not exists as you can see in this jsbin http://jsbin.com/xuhoqibanu/edit?html,console,output , if you try in Chrome both listeners hace the variable but in Firefox the one with addEventListener can't access it. So I would say that add event as global is wrong so I reopened my previous issue #3964 and I'll update this with whatever response got there now.

kileras commented Sep 30, 2015

Well the global event variable is a magic variable that was created a really long time ago as the way of inline event listeners to have access to the event object like:

<body onclick="console.log(event);">

Nowadays thats not used anymore, or shouldn't at least, so the use of that globally defined event variable should be avoided and it's a bad practice and the event listener receive an event object. Also In some situations, as I mention in the other issue, that's even worse as some libraries that modify for whatever reason the event that the listener receives, or create a new one, so it won't necessarily match the global one.

So basically the event is a very specific kind of global variable, created to support some functionality that should not be used at all, magically populated when an event is fired and its used is considered a bad practice, thats why I think it may deserve a specific rule.

But anyway it turns out that while playing with it I discovered that actually event is not really global in all the browsers, it is only in webkit/blink and trident, in Firefox (and potentially old opera didn't test) the event global variable is only defined in inline listener like the one above, the rest of the time it does not exists as you can see in this jsbin http://jsbin.com/xuhoqibanu/edit?html,console,output , if you try in Chrome both listeners hace the variable but in Firefox the one with addEventListener can't access it. So I would say that add event as global is wrong so I reopened my previous issue #3964 and I'll update this with whatever response got there now.

@kileras

This comment has been minimized.

Show comment
Hide comment
@kileras

kileras Oct 5, 2015

Well so back here since the removal of the global didn't get accepted so I think that we totally need a rule to prevent this for happening.

I already explain why I think that this rule is quite specific, its kind of magical properties, the fact that it comes form a old now deprecated practice and it's absence in some browsers. But I must admit that having a rule only for this seems a bit wrong as @nzakas said is kind of similar to #3358 but only applied to globals, which at the end is nothing but a rule to remove globals. I'm not entirely sure thats a good thing either since I don't think that removing globals is going to be a common thing and feels to me that then it may be a problem with the global definition themselves, that's also why I made the suggestion of removing it.

Maybe it can be defined by the fact that is legacy event handling or a browser proprietary extension and we can disable that but I don't know which, if any, other rules may fall in that category so it's not a rule for only one single case

kileras commented Oct 5, 2015

Well so back here since the removal of the global didn't get accepted so I think that we totally need a rule to prevent this for happening.

I already explain why I think that this rule is quite specific, its kind of magical properties, the fact that it comes form a old now deprecated practice and it's absence in some browsers. But I must admit that having a rule only for this seems a bit wrong as @nzakas said is kind of similar to #3358 but only applied to globals, which at the end is nothing but a rule to remove globals. I'm not entirely sure thats a good thing either since I don't think that removing globals is going to be a common thing and feels to me that then it may be a problem with the global definition themselves, that's also why I made the suggestion of removing it.

Maybe it can be defined by the fact that is legacy event handling or a browser proprietary extension and we can disable that but I don't know which, if any, other rules may fall in that category so it's not a rule for only one single case

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Oct 5, 2015

Member

There's a significant difference between removing globals and warning on their use.

If you remove a global, then you change how multiple rules work, and you might not really get the message that you aren't supposed to use it. Existence shouldn't be a proxy for allowable use.

This rule would specifically say, don't use this variable.

Member

nzakas commented Oct 5, 2015

There's a significant difference between removing globals and warning on their use.

If you remove a global, then you change how multiple rules work, and you might not really get the message that you aren't supposed to use it. Existence shouldn't be a proxy for allowable use.

This rule would specifically say, don't use this variable.

@kileras

This comment has been minimized.

Show comment
Hide comment
@kileras

kileras Oct 6, 2015

Sorry that was a poor choice of words, english is not my first language 😄

What I meant was exactly that, removing it in the sense of removing it conceptually from the valid globals by showing a warning like 'Should't use window.event' or something similar.

So any more thoughts at the rule itself? Should be something more generic that handle this situation for any global? Should we keep this a standalone one?

kileras commented Oct 6, 2015

Sorry that was a poor choice of words, english is not my first language 😄

What I meant was exactly that, removing it in the sense of removing it conceptually from the valid globals by showing a warning like 'Should't use window.event' or something similar.

So any more thoughts at the rule itself? Should be something more generic that handle this situation for any global? Should we keep this a standalone one?

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Oct 6, 2015

Member

I think creating no-restricted-globals that lets you specify the global variables to warn on is the best way to go.

Member

nzakas commented Oct 6, 2015

I think creating no-restricted-globals that lets you specify the global variables to warn on is the best way to go.

@nzakas nzakas added accepted help wanted and removed evaluating labels Oct 6, 2015

@platinumazure

This comment has been minimized.

Show comment
Hide comment
@platinumazure

platinumazure Oct 11, 2015

Member

Would this rule be expected to also catch (e.g.) window.event? (Or self.event, or global.event, or...)

Put another way, is there any infrastructure in ESLint which allows rules to examine what identifiers point to an object containing the global scope? And if not, would that need to be created?

Member

platinumazure commented Oct 11, 2015

Would this rule be expected to also catch (e.g.) window.event? (Or self.event, or global.event, or...)

Put another way, is there any infrastructure in ESLint which allows rules to examine what identifiers point to an object containing the global scope? And if not, would that need to be created?

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Oct 12, 2015

Member

No, window.event isn't considered a global variable for these purposes. I really don't want to get into the weeds of what the global object is called in different environments.

Member

nzakas commented Oct 12, 2015

No, window.event isn't considered a global variable for these purposes. I really don't want to get into the weeds of what the global object is called in different environments.

@platinumazure

This comment has been minimized.

Show comment
Hide comment
@platinumazure

platinumazure Oct 12, 2015

Member

I hear that and agree with you 100%. Just wanted to get the question out there.

Member

platinumazure commented Oct 12, 2015

I hear that and agree with you 100%. Just wanted to get the question out there.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Oct 13, 2015

Member

👍

Member

nzakas commented Oct 13, 2015

👍

@zowers

This comment has been minimized.

Show comment
Hide comment
@zowers

zowers Oct 13, 2015

no-restricted-globals that lets you specify the global variables to warn on

to error on - is what I would favor

zowers commented Oct 13, 2015

no-restricted-globals that lets you specify the global variables to warn on

to error on - is what I would favor

@ilyavolodin

This comment has been minimized.

Show comment
Hide comment
@ilyavolodin

ilyavolodin Oct 13, 2015

Member

@zowers error vs. warning is handled on the engine level, and has no bearing on the rule. It's up to the user to set that up.

Member

ilyavolodin commented Oct 13, 2015

@zowers error vs. warning is handled on the engine level, and has no bearing on the rule. It's up to the user to set that up.

@nzakas nzakas changed the title from Add a no-global-event rule to prevent the use of the magic global event variable in browsers to Add a no-restricted-globals rule to prevent the use of the specific globals Dec 15, 2015

This was referenced Dec 15, 2015

@nzakas nzakas changed the title from Add a no-restricted-globals rule to prevent the use of the specific globals to Add a no-restricted-globals rule to prevent the use of the specific globals [$25] Dec 27, 2015

@nzakas nzakas added the bounty label Dec 27, 2015

@rpatil26

This comment has been minimized.

Show comment
Hide comment
@rpatil26

rpatil26 Jan 4, 2016

Contributor

@nzakas and @willklein - Do you think this could be done through #3218 (no-restricted-properties) where object could be "" (or something) to specify global?

Contributor

rpatil26 commented Jan 4, 2016

@nzakas and @willklein - Do you think this could be done through #3218 (no-restricted-properties) where object could be "" (or something) to specify global?

@willklein

This comment has been minimized.

Show comment
Hide comment
@willklein

willklein Jan 4, 2016

@rpatil26, my initial thought is I don't think we should overload that rule for this instance. I'll think this over some more. I guess you could configure no-restricted-properties as it will be implemented, from what I can gather. The tricky part is there are globals that you can call without the window "prefix," which makes me think a dedicated rule would make more sense.

Related to this, I have my own way of addressing this problem. Because there's some performance degradation with certain environments configured, I dropped browser from my environments configuration. Then I whitelisted the globals for document and window since those are all I need, and currently I don't restrict anything called off them, though I could with no-restricted-properties.

willklein commented Jan 4, 2016

@rpatil26, my initial thought is I don't think we should overload that rule for this instance. I'll think this over some more. I guess you could configure no-restricted-properties as it will be implemented, from what I can gather. The tricky part is there are globals that you can call without the window "prefix," which makes me think a dedicated rule would make more sense.

Related to this, I have my own way of addressing this problem. Because there's some performance degradation with certain environments configured, I dropped browser from my environments configuration. Then I whitelisted the globals for document and window since those are all I need, and currently I don't restrict anything called off them, though I could with no-restricted-properties.

@rpatil26

This comment has been minimized.

Show comment
Hide comment
@rpatil26

rpatil26 Jan 4, 2016

Contributor

Thanks @willklein. I think this is to restrict pure globals accessed without reference of document or window.

Let's not overload the rule if it doesn't make sense. Just thought to check.

Contributor

rpatil26 commented Jan 4, 2016

Thanks @willklein. I think this is to restrict pure globals accessed without reference of document or window.

Let's not overload the rule if it doesn't make sense. Just thought to check.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Jan 4, 2016

Member

This rule should not check properties of window, as that's browser specific (and fairly obvious). That can be done with no-restricted-properties if necessary.

We can't use "" or something similar with no-restricted-properties to block globals because we need to check global declarations through escope to pick up environment settings plus explicitly declared globals.

Member

nzakas commented Jan 4, 2016

This rule should not check properties of window, as that's browser specific (and fairly obvious). That can be done with no-restricted-properties if necessary.

We can't use "" or something similar with no-restricted-properties to block globals because we need to check global declarations through escope to pick up environment settings plus explicitly declared globals.

@BenoitZugmeyer

This comment has been minimized.

Show comment
Hide comment
@BenoitZugmeyer

BenoitZugmeyer Feb 24, 2016

Contributor

I am working on this.

Contributor

BenoitZugmeyer commented Feb 24, 2016

I am working on this.

BenoitZugmeyer added a commit to BenoitZugmeyer/eslint that referenced this issue Feb 24, 2016

@nzakas nzakas closed this in 560c0d9 Feb 25, 2016

nzakas added a commit that referenced this issue Feb 25, 2016

Merge pull request #5392 from BenoitZugmeyer/no-restricted-globals
New: no-restricted-globals rule implementation (fixes #3966)

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