-
-
Notifications
You must be signed in to change notification settings - Fork 70
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
Update: Enable ES2017 parsing and globals by default #16
Conversation
I think the 2017 env should be added (i think all years should have an env, no matter what), but i still don’t think this is a safe default. Perhaps when node ships modules, a reasonable set of defaults would be “es5” or “ESM”, but until then i don’t think it would be a good idea to change the default. |
|
||
The current behavior is "noisy by default" -- if ESLint isn't sure what behavior the user wants and its default gets things wrong, an ES5 default makes the error manifest as a false positive rather than a false negative. This has some significant advantages -- a user is much more likely to notice a false positive than a false negative, allowing them to correct the config error. | ||
|
||
However, at this point it appears that most of our users would expect ES6 parsing by default. As a result, the advantage of a false positive over a false negative is outweighed by the fact that making this change would be replace a very large number of false positives with a small number of false negatives. (Using similar reasoning, [eslint/eslint#11105](https://github.com/eslint/eslint/issues/11105) updated `eslint --init` to set `parserOptions.ecmaVersion` to `2018` by default.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The difference here is, a false positive creates noise and friction, but a false negative creates silent breakage. Implicit breakage is never worth convenience, imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that a false negative is worse than a false positive, but I disagree with your assessment that reducing false positives is never worth a smaller increase in false negatives. Aside from inconvenience, I think having too many false positives when starting out could deter someone from using a linter at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @ljharb here.
|
||
However, at this point it appears that most of our users would expect ES6 parsing by default. As a result, the advantage of a false positive over a false negative is outweighed by the fact that making this change would be replace a very large number of false positives with a small number of false negatives. (Using similar reasoning, [eslint/eslint#11105](https://github.com/eslint/eslint/issues/11105) updated `eslint --init` to set `parserOptions.ecmaVersion` to `2018` by default.) | ||
|
||
### The `es6` environment composes poorly with other environments because it sets `parserOptions.ecmaVersion` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m +1 on this as well (itd be nice to separate this, and the 2017 env, from the default change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could theoretically be separated, but it would end up being a breaking change for many more users because people are relying on env: { es6: true }
resulting in a parserOptions.ecmaVersion
value of at least 2015.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is that not also the case for a larger env value? ES2015 isn’t the latest version that added new globals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the only new globals after ES2015 have been Atomics
and SharedArrayBuffer
, and we still don't have an environment containing those. (Aside from this RFC, the issues discussed in eslint/eslint#9109 haven't been resolved.)
|
||
## Drawbacks | ||
|
||
The main downside of this proposal is that it would be a silent breaking change for existing users who only want ES5 parsing/globals. This kind of breaking change is always unappealing, since some users who upgrade might not notice the change until they realize that certain code is allowed when it should have been blocked. However, as fewer and fewer users run their code directly on ES5 environments, the default behavior becomes less and less useful, and I think we've reached a point where it's worthwhile to change the default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That they know of - plenty of human beings still use older browsers to access sites.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Note: in the phrase "run their code directly on ES5 environments", I'm using "directly" to mean "without compilation from something like babel/typescript".)
My assumption is that it's rare for a site with a lot of changing code to "unintentionally" support old browsers as a result of ESLint's default behavior. An ES5-only parser won't protect someone from using, say, the fetch
API, unless they are intentionally avoiding fetch
to improve browser compatibility.
Of course, developers should consciously consider the impact on users of old browsers when deciding what environments to support. But I don't think relying on ESLint's default behavior is a good way to ensure support for older browsers anyway, because it only deals with syntax and the availability of ES globals. People would be better off using something like eslint-plugin-compat
to support old browsers, and a plugin like that can set the appropriate parser options independently of what ESLint does by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still unclear on which users this is meant to help. Yes, ecmaVersion: 5
is the default, but because ESLint requires a configuration file to execute, ESLint very rarely runs without any version information. If we assume that new ESLint users run --init
(which always sets ecmaVersion
and globals to the latest) or extend a shareable config, there are very few remaining users.
Can you clarify who are the users you envision benefiting from this change? It seems like the default ecmaVersion
would only affect users who have a config file that for some reason doesn't include ecmaVersion
somewhere, and I can't imagine that group of users being very large.
This RFC proposes to: | ||
|
||
* Add an `es2017` environment which enables all ES2015 and ES2017 globals. | ||
* Enable the `es2017` environment by default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm against enabling any environments by default. This has the potential to break a lot of existing shareable configs unnecessarily. For example, if a shareable config just has ecmaVersion: 5
, this change would automatically include all of the ES2017 globals and would affect how rules like no-undef
, no-redeclare
, and no-shadow
work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm understanding correctly, by "break a lot of existing shareable configs" do you just mean that new globals would sometimes be present when they weren't before? Presumably, a shareable config that works before could still be extended after this change.
I view this to be the same as changing the default options for a rule, which we occasionally do in a breaking change if we think the new default behavior is more likely to be what a user expects. In the same way, changing the default options for a rule can change the configuration provided by a shareable config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I mean is that a shareable config currently explicitly lays out exactly what globals should exist. People then configure their own config with rules that will use that information to result in a lint-free codebase. By adding new globals by default, we are now changing the expectations because a bunch of rules use those globals to determine whether there is a problem or not.
As a concrete example, perhaps someone created a local variable called Atomics
in their code and has no-shadow
enabled. Right now, they do not have Atomics
defined as a global so their code passes. If we automatically enable Atomics
, then they will get a lint error.
Given that we don't have any easy way to include ES2017 globals currently, I think this would end up being the common case with this change, creating a lot of unnecessary disruption for users.
|
||
* Add an `es2017` environment which enables all ES2015 and ES2017 globals. | ||
* Enable the `es2017` environment by default. | ||
* Change the `es6` environment to no longer enable `parserOptions: { ecmaVersion: 6 }`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also not a fan of this. Once again, we may be breaking a large number of existing shareable configs. If people only have es6: true
, then their configs will start throwing parsing errors with an upgrade.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe a better option than breaking env, is adding a new top level option that wouldn’t imply an ecmaVersion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If people only have
es6: true
, then their configs will start throwing parsing errors with an upgrade.
This doesn't seem like it will be an issue if we change the default parserOptions
value at the same time. (I have a note about this under "Backwards Compatibility Analysis".)
- Users who enable the
es6
env without also configuringparserOptions
will end up with the new default parsing behavior ofparserOptions: { ecmaVersion: 2017 }
, so no new parsing errors will be produced. - Users who enable the
es6
env and also configureparserOptions
will still have their own parser options used, becauseparserOptions
have precedence overenv
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels a bit like trying to pull one dish out of a stack of dishes and hoping the food on top doesn't spill.
And as I pointed out in the previous bullet, I don't think implicitly changing what people think they are configuring is a good idea because it will end up breaking a large number of users without much additional benefit, especially if we add an es2017
environment that doesn't include parserOptions
and includes the additional globals.
* Add an `es2017` environment which enables all ES2015 and ES2017 globals. | ||
* Enable the `es2017` environment by default. | ||
* Change the `es6` environment to no longer enable `parserOptions: { ecmaVersion: 6 }`. | ||
* Change the default value of `parserOptions.ecmaVersion` in `espree` from `5` to `2017`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay with this because it doesn't affect end-users, though I'm not sure what practical value it has.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might as well set it to 2019 if it’s being changed :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The practical effect would be that if a user's config doesn't specify ecmaVersion
, it gets parsed as ES2017 by default rather than ES5. (ESLint doesn't actually pass a default ecmaVersion
to parsers -- the default of ecmaVersion: 5
in ESLint comes from espree.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eslint could start doing so, however, and probably should anyways to insulate users from espree's choices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that we currently don't is an intentional choice -- see here.
|
||
The current behavior is "noisy by default" -- if ESLint isn't sure what behavior the user wants and its default gets things wrong, an ES5 default makes the error manifest as a false positive rather than a false negative. This has some significant advantages -- a user is much more likely to notice a false positive than a false negative, allowing them to correct the config error. | ||
|
||
However, at this point it appears that most of our users would expect ES6 parsing by default. As a result, the advantage of a false positive over a false negative is outweighed by the fact that making this change would be replace a very large number of false positives with a small number of false negatives. (Using similar reasoning, [eslint/eslint#11105](https://github.com/eslint/eslint/issues/11105) updated `eslint --init` to set `parserOptions.ecmaVersion` to `2018` by default.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @ljharb here.
@nzakas Thanks for the feedback.
This would largely affect users who create config files without using |
One option for solving the issue with environments which set Example:
|
Just to summarize my overall thinking here:
|
Closing, as we won't be moving forward with this. |
@nzakas what about adding an es2017 environment? |
Summary
This RFC proposes to:
es2017
environment which enables all ES2015 and ES2017 globals.es2017
environment by default.es6
environment to no longer enableparserOptions: { ecmaVersion: 6 }
.parserOptions.ecmaVersion
inespree
from5
to2017
.Related Issues