-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Every single ECMAScript edition should have an "env" option #9109
Comments
Thanks for opening a new issue. This proposal seems reasonable to me. I'm a bit concerned that this could cause more confusion about the distinction between What would you expect to happen to the parser options if someone specifies multiple envs at once? env:
es2015: true
es2017: true |
I would expect the same as if you currently set |
I'm not sure the union of |
Ah, I misunderstood. It should set the latest parserOptions available - or, combining multiple ES env values should be an error. |
In reverse, should setting a
I don't think it's too late to think about deprecating one of them. If the goal is to "do the Right Thing™" without making users worry about it, the two options aren't really needed, unless you think there is a legitimate case where the user would want the parser to recognize certain symbols, but not actually allow the linter to accept those symbols into its rule set. |
I think it's important to distinguish between the "symbols" we're referring to here. There are use cases for wanting one but not the other. For example, if a library is developed in ES2017 but is transpiled down to ES5 as a build step, its authors would want to allow ES2017 syntax because that syntax will be transpiled away. However, they would want to disallow ES6 globals, because those might not be present when the library is running. Similarly, if an application uses a polyfill without transpiling, then its developers might want to allow ES6 globals (since those will be present from the polyfill), but disallow ES6 syntax (since that will cause a parsing error in ES5 environments). |
The conflict of "which parserOptions gets set" is already an issue if i'm using es2015 and es2017, because both have new globals and new syntax. I think it should be solved separately from this issue, which is only asking for "es2016" to exist (and future editions as well) even if they don't introduce new globals. |
The es2017 env doesn't exist yet, so it's not already a problem. I think the "which parserOptions gets set" question is a blocker for both issues. |
Gotcha. In that case, since ES2017 was released in June, the problem is late being resolved by either 2 or 14 months, depending on the outcome of this issue :-/ I would not expect env to set parserOptions; i would expect parserOptions to set env; however I think it would be fine if neither set the other, and that might be the best approach here. |
This loosely relates to #8556. tl;dr: Part of the current behavior for overriding edit: There was also some discussion about this in #8291.
I'm slightly averse to doing this because parser options are supposed to be parser-specific. For example, I think
You may be right. Unfortunately, we probably can't ever remove the link between It's worth noting that |
The Edit: To clarify, my above comments are only referring to |
Thanks for your thoughts @skylize. I have a few questions:
|
@not-an-aardvark in the next semver-major, it could be on by default; but for migration, it would be off by default? |
What do you mean by "off by default"? Isn't that the same thing as not implementing the feature until the next semver-major release? Or would there be another configuration flag to enable it? |
Ah, I'm still seeing this issue as for "there's an env for every edition" - I don't particularly care much what the implicit auto-opt-in behavior is :-) |
What's the use for using specifying es6 parser to write es5 code? That seems counter-intuitive to me and likely to cause plenty of bugs even without this change. If there is valid reason to use such settings, then I guess we'll need an |
This isn't really what parsers are for, and I don't think we should couple the two behaviors. Parsers just accept source text and output an AST -- they don't have anything to do with globals.
See #9109 (comment). It's fairly common for a library to write ES6 code and transpile it to ES5 with babel. Libraries like this can use ES6 syntax (since it will be transpiled anyway at build time), but they can't use ES5 globals (since they won't be available at runtime).
I don't think adding another env would help here. When multiple envs are enabled, all of the globals from both envs are applied.
What are we deprecating? It seems like this is just switching from one behavior to another behavior for a given configuration, so it doesn't seem like there is any configuration that would end up being deprecated. |
Okay, well that's fine. If the parser is not concerned with globals and ignores
Okay, that usage makes sense. However it still seems pretty clear to me that An
We're deprecating an implied setting, in favor of an explicit one. "Setting globals to ES5 by not specifying an ES version in We could introduce |
I'm unsure about this, especially given that
I see what you mean, thanks for clarifying. So it would be less of a warning about a particular config behavior being deprecated, and more of a warning that the given config behavior will soon mean something else. However, I'm unconvinced that this is actually better than just keeping parser options and globals completely separate. Conceptually, the options control two different things, so it seems reasonable to me that the two options should also be independent, rather than sometimes subtly depending on each other. To throw another element into the mix: At some point in the future, we are probably going to enable |
Another possibility: Could we completely decouple envs and parser options, and instead create and publish shareable configs that will emulate those? (e.g., Maybe envs are taking on too much responsibility and should be reimagined as global-sets, and we should use shareable configs as the convenience pieces. |
I'm not saying that a setting called
From your perspective, seeing these two things as clearly separate makes perfect sense because you understand the internal implemenation of From an average user's perspective, it is "Why do I need to set es6 twice? )%&#_%()$&#)%)#!!!!!!!! put es2017 just like the docs, why won't it understand me?!?" Average joe has no idea how it works. He just writes up a settings file after scouring the docs, then feeds them into a magic box that spits out lints and expects it to work. The delineation between parser and environment has very little meaning until you try to look under the hood. They are just arbitrary names under which to apply some configs. Underneath the hood, sure decouple them to your hearts content. I'm sure that makes things easier to understand and easier to maintain. This is basically just sugar for the most obvious cases. |
I'm unconvinced about this. This is not just a concern about maintainability -- there are very common cases where users need to understand the difference between parser options and globals. (This applies when developing almost any library with Babel -- I don't think it's as much of an edge case as you're describing). I like @platinumazure's solution of treating this like a config, since it allows new users to set a single flag without muddying the difference between parser options and envs. Rather than making them shareable, another option would be provide them as builtin configs like |
You're right, I'm probably overstating this by calling it an edge case. The solution I offered still leaves the decoupled nature of
I'm not a big fan of adding more places to put options here. We have enough options availiable already if we can just shift gears slightly to interpret them properly.
I don't like this quite as well as the solution I suggested, but I think I could get behind it. This accomplishes the goal of reducing es version to a single setting for new users and simple cases, within an already existing namespace. 👍 I assume that explicitly setting |
Yes, that's part of the reason I recommended shareable configs (though I can get behind builtin configs as well). We also allow multiple inherited configs, as well. In fact, a config of "eslint:es6" (or whatever) would serve as the single setting that |
Can you elaborate on what this means? |
@skylize Basically, you can extend from multiple configurations: {
"extends": ["eslint:es6", "eslint-config-someconfig", "eslint-config-someotherconfig"],
"parserOptions": {
"ecmaVersion": 5
}
} Or your shared configs (eslint-config-someconfig, eslint-config-someotherconfig in the example above) could extend from "eslint:es6", given time to upgrade. In the config above, parserOptions would be overwritten so you would be parsing ES5 code but having the ES6 globals available. (Not a common use case, I'm sure, but good for illustration.) At the moment, I think there might be an issue where envs cannot actually be disabled by configs extending other configs-- but I think the reason was because of how some envs set parserOptions etc., so it wasn't always clear what should happen when an env was disabled. If an env is now just a set of globals and we use shareable or builtin configs to indicate ES6 parsing and globals simultaneously, we might be able to fix that too. Not 100% sure though. |
@platinumazure Thanks for that description. Is there any good reason besides legacy for having these coupled internally? Based on discussion so far, it seems to me these should definitely be fully decoupled under-the-hood. Any apparent coupling should be created later by an overlay of some kind to benefit specific use cases, whether that's configs, or sugar, or whatever. |
@skylize I'm not 100% aware of the history but I think it's honestly just legacy. We took some of our original direction from JSHint which did all sorts of crazy crap like this, and shareable configurations were (compared to envs) a more recent innovation. To be completely honest, I'm surprised nobody has thought to create configurations to replace the more tightly coupled envs before. Maybe @not-an-aardvark and others will come up with a better way forward (especially if we can find a way to reduce pain of upgrading to our users). But I'm hoping that the upgrade pain will be outweighed by the significant reduction in confusion that I think this solution would promise. |
I suggested builtin configs because it seems like if we're worried about the UX impact of needing to set 2 config flags rather than 1, then needing to install a separate npm package would be a much bigger UX barrier. I want to refocus the discussion for a second, with two questions:
The second question seems important for determining how to proceed with this issue. If we're planning to decouple |
I feel like from this discussion and others I've skimmed across in the past, it's pretty clear that the coupling causes a lot of unnecessary pain, and the sooner we can be rid of it, the better for everybody. I think there are ways we could make the OP @ljharb happy without that decoupling, but it seems like just another band-aid on something that's due for surgery. |
I'm not sure I understand the reason for wanting to decouple I think the other point was already dropped earlier in the discussion, but I want to put my vote strongly against having |
@skylize for the record, I'm the original original OP 😄 Having followed the discussion for a while, I think a middle ground can be reached. I agree with @not-an-aardvark that having the two decoupled can be extremely valuable to certain users. I also agree with @skylize that for most users, they just want to set the one thing and be done with it (i.e., they don't distinguish between using say es2018 globals and es6 syntax - usually because they have something like babel transpiring the syntax anyways). I propose a middle ground solution: I think part of the problem is that today,
I think this allows both the flexibility for power users that @not-an-aardvark desires, and the simplicity for average users that @skylize desires. This does break the way |
Thanks for your thoughts @MatthewHerbst. To me, it seems like adding another option will just complicate things even more, because there will be more cases/defaults to deal with. My goal in discussing this is to make the options conceptually simpler, not to make them more flexible than they currently are. (Users can already control globals and parser options separately by using a combination of That's why I like (For what it's worth, there is already a |
Ah, got it. I was under the impression that you were concerned the changes being proposed would make the existing options less flexible.
I mean, having If we agree to go down that route, I'm more than happy to write that PR (I think two are needed, one for this lib, and one for the |
@MatthewHerbst Sorry for any confusion about who is OP. I am still not seeing your name on the opening post of this particular thread, but I'll take your word for it. ;😉
I think @not-an-aardvark's "What will happen in this case?" questions are mostly about ensuring we take all the implications of any changes into account when making a decision.
I agree with this strongly. Conceptual simplicity certainly improves maintainability, and it tends to provide flexibility as a natural byproduct. |
Unfortunately, it looks like there wasn't enough interest from the team Thanks for contributing to ESLint and we appreciate your understanding. |
Can we re-open this? I think there is plenty of interest, but we're waiting on some upstream decision. For example, someone commenting on this PR in |
Yes, let's please reopen this. It's very confusing to have a discontinuous set of editions. |
@ljharb any thoughts on the linked PR? |
@MatthewHerbst not really, no; i don't use that package. |
ESLint uses that package. It's my understanding that ESLint is the primary consumer of that package. I think solving this issue greatly involves the |
If it is decided to keep the association between When It would set For example, setting the version number: {
"env": {
"ecmaVersion": 9
}
} would output {
"env": {
"ecmaVersion": 9
},
"parserOptions": {
"ecmaVersion": 9
}
} Using the year: {
"env": {
"ecmaVersion": 2018
}
} would output {
"env": {
"ecmaVersion": 2018
},
"parserOptions": {
"ecmaVersion": 2018
}
} |
es7/es2016 adds no new globals - that means that there's no "env" option for it.
This is confusing, because users don't know these details, and don't want to have to think about it - a user should be able to set their
ecmaVersion
andenv
to, say, "es2016" or "es2017" and have eslint do the Right Thing™.If there are no new globals in a year, then great - es2016's globals are the same as es2015's - but that's not something the user writing the eslint config should have to care about.
(opening a new issue per #8453 (comment))
The text was updated successfully, but these errors were encountered: