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.

Sign up for GitHub

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

Jump to bottom

Docs: Added note about Node/CJS scoping to no-redeclare (fixes #8814) #8820

Merged
merged 2 commits into from
Jun 28, 2017
Merged

Docs: Added note about Node/CJS scoping to no-redeclare (fixes #8814) #8820

merged 2 commits into from
Jun 28, 2017

Conversation

platinumazure
Copy link
Member

What is the purpose of this pull request? (put an "X" next to item)

[x] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

See #8814 (but scroll down a bit).

What changes did you make? (Give an overview)

Added a note to no-redeclare docs explaining that Node/CommonJS module scoping means "global variables" are not actually global, so the "builtinGlobals" option will not work as expected. The note also recommends the use of no-shadow in that case, which would hopefully satisfy users' needs in those cases.

Is there anything you'd like reviewers to focus on?

Is the new paragraph clear? Could it be improved?

@eslintbot
Copy link

Thanks for the pull request, @platinumazure! I took a look to make sure it's ready for merging and found some changes are needed:

  • The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

@eslintbot
Copy link

Thanks for the pull request, @platinumazure! I took a look to make sure it's ready for merging and found some changes are needed:

  • The commit summary must be 72 characters or shorter. Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

@platinumazure platinumazure changed the title Added note about Node/CommonJS scoping to no-redeclare (fixes #8814) Docs: Added note about Node/CommonJS scoping to no-redeclare (fixes #8814) Jun 28, 2017
@platinumazure platinumazure added accepted There is consensus among the team that this change meets the criteria for inclusion documentation Relates to ESLint's documentation rule Relates to ESLint's core rules labels Jun 28, 2017
@platinumazure platinumazure changed the title Docs: Added note about Node/CommonJS scoping to no-redeclare (fixes #8814) Docs: Added note about Node/CJS scoping to no-redeclare (fixes #8814) Jun 28, 2017
not-an-aardvark
not-an-aardvark approved these changes Jun 28, 2017
View reviewed changes
docs/rules/no-redeclare.md Outdated
@@ -50,3 +52,9 @@ var top = 0;
```

The `browser` environment has many built-in global variables (for example, `top`). Some of built-in global variables cannot be redeclared.

Note that when using the `node` or `commonjs` environments (or `ecmaFeatures.globalReturn`, if using the default parser), the top scope of a program is not actually the global scope, but rather a "module" scope. When this is the case, declaring a variable named after a builtin global is not a redeclaration, but rather a shadowing of the global variable. In that case, the `no-shadow` rule with the `"builtinGlobals"` option should be used.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Would it be worth linking to the no-shadow rule in the last sentence?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think so. I'll push a new commit, thanks.

@eslintbot
Copy link

Thanks for the pull request, @platinumazure! I took a look to make sure it's ready for merging and found some changes are needed:

  • The commit summary must be 72 characters or shorter. Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

@not-an-aardvark not-an-aardvark merged commit 84d921d into master Jun 28, 2017
@not-an-aardvark not-an-aardvark deleted the issue8814 branch June 28, 2017 07:41
This was referenced Jul 9, 2017
Closed
Closed
Closed
Closed
Merged
@HomeZenBot HomeZenBot mentioned this pull request Jul 10, 2017
Merged
@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.
Reviewers

@not-an-aardvark not-an-aardvark not-an-aardvark approved these changes

Assignees
No one assigned
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion documentation Relates to ESLint's documentation rule Relates to ESLint's core rules
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@platinumazure @eslintbot @not-an-aardvark