New: no-global-assign rule (fixes #5358)

[mysticatea]

Fixes #5358.

And I added a note into docs/user-guide/configuring.md.

[eslintbot]

LGTM

[mention-bot]

By analyzing the blame information on this pull request, we identified @nzakas, @scriptdaemon and @pedrottimark to be potential reviewers

[eslintbot]

LGTM

[nzakas]

I'd suggest something like:

JavaScript environments contain a number of built-in global variables, such as window in browsers and process in Node.js. In almost all cases, you don't want to assign a value to these global variables as doing so could result in losing access to important functionality. For example, you probably don't want to do this in browser code:

window = {};

While examples such as window are obvious, there are often hundreds of built-in global objects provided by JavaScript environments. It can be hard to know if you're assigning to a global variable or not.

[nzakas]

Code looks good, just some documentation things to clear up.

[mysticatea]

Oooh, now I found that this rule has existed already -- no-native-reassign.
That rule and this rule does the same thing.

I'll close this PR after someone double-check it. Then I will send a PR to update document.

@nzakas Thank you for the correction!

[platinumazure]

@mysticatea That rule seems to focus on reassignments- I'm not sure there is a rule to cover assignments of new global variables. Then again, I only glanced at the source and escope is one of my weak areas, so I can't tell for sure. But I thinking there is probably a need for this rule.

[mysticatea]

@platinumazure Actually, the rule warns all modifications of read-only global variables.

[platinumazure]

Yeah, I'm talking about new variables:

MyObject = {};

That's not warned by no-native-reassign, is it?
On Jun 13, 2016 7:34 PM, "Toru Nagashima" notifications@github.com wrote:

@platinumazure https://github.com/platinumazure Actually, the rule
warns all modifications of read-only global variables.

[image: image]
https://cloud.githubusercontent.com/assets/1937871/16027700/129f3a14-3213-11e6-8f71-a25146cbff66.png

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6395 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AARWetO-xbxvxt5zftYW2IyEL039Jx1Wks5qLfcrgaJpZM4Iz243
.

[mysticatea]

@platinumazure Yeah, it's warned by no-undef rule.

[nzakas]

Oh interesting! It looks like this rule actually does more than no-native-assign because it applies to all globals, not just a subset. It might be better to keep this one and deprecate no-native-assign?

[mysticatea]

because it applies to all globals, not just a subset.

Both no-native-reassign and this no-global-assign are warning only read-only globals.

• eslint/lib/rules/no-native-reassign.js

  Line 70 in c2b5277

  if (variable.writeable === false && exceptions.indexOf(variable.name) === -1) {

• https://github.com/eslint/eslint/pull/6395/files#diff-95bb9c0db2e3c74471f94ee2d664ee4cR32

I don't oppose renaming :)

[nzakas]

Oops, I missed that! I don't think renaming is a good idea -- it's a breaking change for no good reason.

Should we close this?

[platinumazure]

I don't know- I think it's confusing. "no-native-assign", to me, means you
can't overwrite predefined ("native") globals like window, Object, etc.,
and I can't tell from the name (as an end user) that it flags writes to all
declared globals (or any global).
On Jun 16, 2016 10:23 AM, "Nicholas C. Zakas" notifications@github.com
wrote:

Oops, I missed that! I don't think renaming is a good idea -- it's a
breaking change for no good reason.

Should we close this?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6395 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AARWehynmyHEdkhCoO1nco412L7l3SGyks5qMWqHgaJpZM4Iz243
.

[nzakas]

@platinumazure it's obviously a bit confusing, hence why this PR exists. But is it really worth a breaking change? I don't think so, but this should be discussed on an issue, not a PR.

[mysticatea]

Closing as I agree with @nzakas .
I will send a PR to update documents later.