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

New rule: Prevent/warn about the usage of window in favor of self. #8229

Closed
dryajov opened this issue Mar 10, 2017 · 7 comments
Closed

New rule: Prevent/warn about the usage of window in favor of self. #8229

dryajov opened this issue Mar 10, 2017 · 7 comments
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion triage An ESLint team member will look at this issue soon

Comments

@dryajov
Copy link

dryajov commented Mar 10, 2017

Please describe what the rule should do:
Prevent/warn about the usage of window in favor of self, with self being a proxy to the current global context and should work across all environments in the browser.

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

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

  • Enforces portability across non DOM environments

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

// should warn/error on use of window and suggest usage of self instead
if (window.crypto && window.crypto.getRandomValues) {
    // Modern browsers
    Rand.prototype._rand = function _rand(n) {
      var arr = new Uint8Array(n);
      window.crypto.getRandomValues(arr);
      return arr;
    };
  } else if (window.msCrypto && window.msCrypto.getRandomValues) {
    // IE
    Rand.prototype._rand = function _rand(n) {
      var arr = new Uint8Array(n);
      window.msCrypto.getRandomValues(arr);
      return arr;
    };
  } else {
    // Old junk
    Rand.prototype._rand = function() {
      throw new Error('Not implemented yet');
    };
  }

===============

// should not warn/error on the usage of self
if (self.crypto && self.crypto.getRandomValues) {
    // Modern browsers
    Rand.prototype._rand = function _rand(n) {
      var arr = new Uint8Array(n);
      self.crypto.getRandomValues(arr);
      return arr;
    };
  } else if (self.msCrypto && self.msCrypto.getRandomValues) {
    // IE
    Rand.prototype._rand = function _rand(n) {
      var arr = new Uint8Array(n);
      self.msCrypto.getRandomValues(arr);
      return arr;
    };
  } else {
    // Old junk
    Rand.prototype._rand = function() {
      throw new Error('Not implemented yet');
    };
  }

Why should this rule be included in ESLint (instead of a plugin)?
There is a large surface of non DOM related APIs being added to browsers that work across DOM and non DOM (WebWorker/ServiceWorker) environments. In many cases window is used as a reference to the global context to test for the presence of those features which makes it non-portable across non DOM environments. As we move towards browsers with more and more capabilities beyond the DOM, this is going to become more of a problem. An eslint rule should alleviate the transition and enforce a very important best practice, having it as a plugin would mean less penetration and adoption across the board.

@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label Mar 10, 2017
@not-an-aardvark
Copy link
Member

Thanks for the rule suggestion. To continue the Gitter discussion that led to this issue, you can already use no-restricted-globals to enforce this in the general case, so I don't think it's a good idea to have an entirely new rule just for window versus self. I think enhancing no-restricted-globals to allow custom error messages would be a good idea, though.

@dryajov
Copy link
Author

dryajov commented Mar 10, 2017

I'm entirely for that and indeed it seems to be the best course of action. After all the intent here is to enforce a best practice, if this enhancement would allow for that I see no reason for not doing it, plus the obvious added benefit is that it can be used in other similar scenarios. 👍

Should this issue be kept open to move this effort forward or would a new one be required?

@platinumazure
Copy link
Member

@dryajov If you wouldn't mind editing the first post and title to conform with your new proposal, we can keep using this issue. Thanks! 😄

@platinumazure
Copy link
Member

platinumazure commented Mar 13, 2017

@dryajov I do apologize, I wasn't clear. I'd love to see a proposal for a rule change so that no-restricted-globals can accept messages. Maybe it could be structured similar to no-restricted-properties. That way you get your custom message without us needing to support another rule. 😄

Note that you can also implement a specific no-window rule as a plugin rule, if you want. I just don't see it entering ESLint core.

Oops, I thought the issue was already edited, but it hasn't been. 😄

@dryajov
Copy link
Author

dryajov commented Mar 13, 2017

@platinumazure , I think there is no confusion, im going to enhance no-restricted-globals to take a custom message for globals, which is what has been suggested. I just haven't gotten around changing this issue, will try to get to it today. :)

@platinumazure
Copy link
Member

@dryajov Haha, that's my mistake then, I thought you had already edited the issue. Sorry about that!

@dryajov
Copy link
Author

dryajov commented Mar 23, 2017

closing in favor of #8315

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion triage An ESLint team member will look at this issue soon
Projects
None yet
Development

No branches or pull requests

4 participants