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

CommonJS recommendation + `global` variable #4728

Closed
isiahmeadows opened this Issue Dec 17, 2015 · 9 comments

Comments

Projects
None yet
4 participants
@isiahmeadows
Copy link

isiahmeadows commented Dec 17, 2015

In http://eslint.org/docs/user-guide/configuring#specifying-environments, it specifies to use commonjs, not node for Browserify/Webpack/etc. The most common CommonJS implementations have a global variable, even though it's not mandated in the CommonJS spec, mostly following Node's lead. In browsers, global is an alias for window.

Could this recommendation be either fixed/clarified on the website or patched in ESLint proper?

(cross-posted in eslint/eslint.github.io#169, as it applies there, also)

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Dec 17, 2015

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

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 17, 2015

I'm not sure what you're requesting here. The commonjs environment was created specifically for this purpose, are you saying it's missing global?

@isiahmeadows

This comment has been minimized.

Copy link
Author

isiahmeadows commented Dec 17, 2015

Yes

On Thu, Dec 17, 2015, 11:03 Nicholas C. Zakas notifications@github.com
wrote:

I'm not sure what you're requesting here. The commonjs environment was
created specifically for this purpose, are you saying it's missing global?


Reply to this email directly or view it on GitHub
#4728 (comment).

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 18, 2015

So the solution would be to add global to the CommonJS environment rather than change the documentation. We can fix that.

@nzakas nzakas added bug core accepted and removed triage labels Dec 18, 2015

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Dec 18, 2015

No, we shouldn't do this. It's easy to add a global named global. It should not be implicitly added for everyone using CommonJS.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 18, 2015

@michaelficarra why not? If every CommonJS environment provides global, why should we force people to always add it?

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Dec 18, 2015

A CommonJS environment is not required to have it. One may drop it, or a new one without it may become popular. Given that System.global will likely soon be a thing, I don't think this is too unlikely.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 18, 2015

The fact is that commonjs environments that support global currently dominate the landscape. Given the state of current environments, it seems more likely that any new ones would have to support global for compatibility (even of that just means global = System.global).

I'd rather help people today rather than worry about a future where something becomes popular and doesn't support it.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Jan 13, 2016

The globals module has been updated. This only needs the package to be updated for the fix.

ilyavolodin added a commit that referenced this issue Jan 14, 2016

Merge pull request #4942 from eslint/issue4728
Upgrade: Globals to ^8.18.0 (fixes #4728)

@eslint eslint bot locked and limited conversation to collaborators Feb 6, 2018

@eslint eslint bot added the archived due to age label Feb 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.