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

no-undef does not catch Promise #4085

Closed
pmcelhaney opened this Issue Oct 8, 2015 · 19 comments

Comments

Projects
None yet
8 participants
@pmcelhaney
Copy link
Contributor

pmcelhaney commented Oct 8, 2015

Update (from DelvarWorld):

Since this is the first google result for "eslint promise is not defined" I feel like it would be helpful to explicitly state the solution to this error here:

add "env": { "es6": true } to your .eslintrc.


The no-undef rule allows references to the global variable "Promise" even if it's not marked as a global (which is problematic because IE does not have a global Promise.)

Here's a test case, which can be added to the invalid section of no-undef.js.

        { code: "new Promise();", errors: [{ message: "\"Promise\" is not defined.", type: "Identifier"}] },

If the bug is accepted I'm happy to create a patch.

ESList version: 1.6.0 (ca3cc88)

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Oct 8, 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.

@pmcelhaney

This comment has been minimized.

Copy link
Contributor Author

pmcelhaney commented Oct 8, 2015

Seems the root of the problem is in globals.json

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Oct 8, 2015

All the built-in globals from ECMAScript are considered to be present in ESLint. Getting into per-browser lists of globals is an impossible maintenance nightmare.

globals was split out from this project a while ago, so it's not necessarily a bug there. It was an intentional decision.

@pmcelhaney

This comment has been minimized.

Copy link
Contributor Author

pmcelhaney commented Oct 9, 2015

It's possible to specify es6 as an environment. If that setting is not present why do we assume es6 builtins are present?

Wouldn't it be better to take es5 as the baseline?

As is stands, there's no configuration that allows me to not treat Promise as a global. As developers start getting more familiar with ES6 features but environments in common use don't support all of those features it's going to become a bigger problem.

For the sake of argument, if I convince globals.json to add es5builtin as a separate environment setting -- without altering the existing built in list -- would you consider having eslint default to that list (when es6 is not included in eslint's env config)?

@pmcelhaney

This comment has been minimized.

Copy link
Contributor Author

pmcelhaney commented Oct 9, 2015

@nzakas You asked if we could continue the discussion here. I don't understand why the resistance to having a list of ES5 builtins. Is the list not finalized at this point?

https://es5.github.io/#x15

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Oct 9, 2015

I think you're reading into my statements more than is there. Keep in mind that we have a lot of users and this would be a breaking change, so we need to be very careful in considering the situation.

First, you need to understand how ESLint works. In all cases, no matter what you specify, we use the builtin globals. These are considered to be things provided by the runtime, and since every runtime is different, we made the decision to go with the superset. You can't opt-out of these builtin globals, which is why adding a builtin-es5 to globals would have zero effect on ESLint. You could opt-in to builtin-es5 but that wouldn't opt you out of builtin in general. So the end state for ESLint would be no different.

The only real option to consider is changing builtin to be ES5-only and adding an es6 group that has the ES6-only additions. Then, when you specify the environment as es6, you would get those globals in addition to the builtin ones.

We included all ES6 globals in builtin because we felt like that would best prepare people for the coming ES6 changes. In a JS engine, you aren't going to be able to opt-out of ES6, and so we felt it was important to give people a heads up that the identifiers they were using would cause problems in ES6 environments.

So the question here is whether it makes sense to optimize for ES5 environments (the few remaining) or ES6 environments (growing every day).

As I'm hoping you can see, this is not a straightforward decision.

@pmcelhaney

This comment has been minimized.

Copy link
Contributor Author

pmcelhaney commented Oct 9, 2015

The only real option to consider is changing builtin to be ES5-only and adding an es6 group that has the ES6-only additions. Then, when you specify the environment as es6, you would get those globals in addition to the builtin ones.

That's exactly what I'm proposing. Since backward compatibility is important, we can make the ES6-only builtins opt-out rather than opt-in.

At the end of the day, this issue will be resolved when the use case involving Promise is supported one way or another. I'm okay with adding some configuration options or sacrificing a bit of performance. (Of course, I'm assuming if we solve the problem for "Promise" we'll also solve it other ES6-only built-ins; a special case for one variable would be silly.)

We included all ES6 globals in builtin because we felt like that would best prepare people for the coming ES6 changes. In a JS engine, you aren't going to be able to opt-out of ES6, and so we felt it was important to give people a heads up that the identifiers they were using would cause problems in ES6 environments.

That's a good thing with respect to no-shadow-restricted-names. Definitely keep that.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Oct 12, 2015

That's exactly what I'm proposing. Since backward compatibility is important, we can make the ES6-only builtins opt-out rather than opt-in.

I'm generally against creating options that are at odds with every other option we have. For every environment, you must opt-in to using it, so having a single one that is opt-out doesn't make sense.

Again, given the rate at which ES6 is being adopted, I'm having a hard time rationalizing that we should anything to optimize for ES5 environments, since in all likelihood we'd just be backing out those changes in the not-too-distant future.

@eslint/eslint-team any other thoughts?

@pmcelhaney

This comment has been minimized.

Copy link
Contributor Author

pmcelhaney commented Oct 12, 2015

Okay, let's talk about what the option might actually look like.

/*eslint-env es6 */
 var p = new Promise(); // This is okay. It's ES6 code so it's safe to use Promise.
 Promise = 'x'; // This is not okay.

/*eslint-env es5 */
var p = new Promise(); // This is not okay. Promise may not exist in some environments that this code is supposed to support. (Looking at your, IE11.)
Promise = 'x'; // Also not okay. Possibly shadowing a built-in object. 

The "es6" environment option already exists. The only change to the API would be to add an "es5" option.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Oct 13, 2015

Now you're talking about a special case where "es5" environment undoes what is already done by default. This isn't possible unless we create a special case for "es5", which is definitely not an option.

@pmcelhaney

This comment has been minimized.

Copy link
Contributor Author

pmcelhaney commented Oct 13, 2015

How about adding an assume-no-globals option? With that option enabled, ESLint would not add any global variables that are not explicitly included via "global" or "env".

There would still be an "es5" environment option and it would be purely additive, just like the other "env" options.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Oct 14, 2015

I think I've achieved some enlightenment around this issue.

First, the only reasonable approach that doesn't require a lot of hacks is to remove ES6 globals from builtin into an es6 environment. This will work as-is for anyone currently using the es6 environment, so it will only affect those who are not.

This is still a breaking change, so we'd have to wait for 2.0.0 to do it.

Ultimately, I had to meditate on the original issue description and came to the conclusion that missing an undefined identifier is a Bad Thing(TM) for a linter to do.

So, we can make this change, it will just have to wait for 2.0.0.

@nzakas nzakas referenced this issue Oct 14, 2015

Closed

Roadmap: 2.0.0 #3561

11 of 11 tasks complete

@nzakas nzakas added this to the v2.0.0 milestone Oct 14, 2015

@sindresorhus

This comment has been minimized.

Copy link
Contributor

sindresorhus commented Oct 19, 2015

First, the only reasonable approach that doesn't require a lot of hacks is to remove ES6 globals from builtin into an es6 environment.

There's also the option of marking this as won't fix. I'm strongly against this. JavaScript is version-less. I really don't think we should optimize for the past (ES5).

Why are we even discussing this based on one users' request. Shouldn't we wait until more people request this or at least shares their opinion.

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Oct 19, 2015

Completely agree with @sindresorhus. That's exactly what I've been thinking as I've followed this thread.

@pmcelhaney

This comment has been minimized.

Copy link
Contributor Author

pmcelhaney commented Oct 19, 2015

Why wouldn't a linter for a programming language support the latest version of the language specification?

I understand that you want to be forward-looking and completely support that sentiment. I also believe it's possible to take a step forward while having one foot firmly planted in the ground.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Oct 19, 2015

@sindresorhus we make a lot of fixes based on one user's request, it's not the number of people with the problem that matters, it's the severity of the problem.

In this case, we have a situation where we're getting a false negative that is pretty nasty (ESLint is failing to detect an undefined variable). Further, we have zero options for allowing individual users to fix the problem. Right now, there is no way to get var x = new Promise() to say "Promise is undefined", and that's something I'm not comfortable with, especially as I'm still peresonally supporting a web application that needs to target IE9 at work.

Keep in mind, we're also talking about environments like MongoDB, wsh, AppleScript, etc., that might lag very far behind other JS environments in terms of updating to ES6.

Realistically, such a change will have minimal impact on current ESLint users. Most are already using the es6 environment to opt-in to ES6 functionality. At some point in the future (ESLint 3, ESLint 4), we'll likely end up making ES6 the default and give people some way to opt-out if they don't want it.

@pmcelhaney the latest version is edition 6, not 5.1.

@btmills

This comment has been minimized.

Copy link
Member

btmills commented Dec 6, 2015

Working on this.

btmills added a commit that referenced this issue Dec 6, 2015

btmills added a commit that referenced this issue Dec 6, 2015

btmills added a commit that referenced this issue Dec 6, 2015

@btmills btmills closed this in 460ec05 Dec 7, 2015

nzakas added a commit that referenced this issue Dec 7, 2015

Merge pull request #4621 from eslint/issue4085
Breaking: Remove ES6 global variables from builtins (fixes #4085)
@AndrewRayCode

This comment has been minimized.

Copy link

AndrewRayCode commented May 22, 2016

Since this is the first google result for "eslint promise is not defined" I feel like it would be helpful to explicitly state the solution to this error here:

add "env": { "es6": true } to your .eslintrc.

@bfred-it

This comment has been minimized.

Copy link

bfred-it commented Aug 17, 2016

Alternatively, define the global in .eslintrc:

{
  "globals": {"Promise": true}
}

balupton added a commit to bevry/base that referenced this issue Nov 1, 2016

@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.
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.