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

Changing our parser options/ecmaFeatures #4641

Closed
nzakas opened this Issue Dec 8, 2015 · 18 comments

Comments

Projects
None yet
@nzakas
Member

nzakas commented Dec 8, 2015

As part of looking at better supporting other parsers, I've been thinking a lot about how we're configuring language features. Right now, everything is jam-packed into ecmaFeatures, something that only Espree supports, and it would be nice to create a more parser-agnostic way of configuring these features.

This proposal is not fully fleshed out, just looking to get some eyes on it.

Proposal

  1. Remove all ecmaFeatures that are specific to ES6. Replace with ecmaVersion. Things like globalReturn, jsx, and experimental features would remain. This would also be a change in Espree.
  2. Introduce sourceType that is "script" by default and can be set to "module".
  3. Move these fields into a new parserOptions field in configuration.

Basically, we'd go from this:

        ecmaFeatures:
            arrowFunctions: true
            blockBindings: true
            regexUFlag: true
            regexYFlag: true
            templateStrings: true
            binaryLiterals: true
            octalLiterals: true
            unicodeCodePointEscapes: true
            superInFunctions: true
            defaultParams: true
            restParams: true
            forOf: true
            objectLiteralComputedProperties: true
            objectLiteralShorthandMethods: true
            objectLiteralShorthandProperties: true
            objectLiteralDuplicateProperties: true
            generators: true
            destructuring: true
            classes: true
            spread: true
            newTarget: true

To this:

parserOptions:
  ecmaVersion: 6
  sourceType: "module"    # if you want

Rationale

  • ecmaVersion is supported already by escope, estraverse, and Acorn
  • sourceType is supported already by escope, estraverse, Esprima, Espree, and Acorn
  • Potential for some perf gains by eliminating a ton of if checks from Espree
  • Most people are likely using es6 environment to enable all of ES6, so as long as the es6 environment switches parser options, then it should be no change for those users.
  • Those who are using ecmaFeatures to disable specific features they don't like can use no-restricted-syntax to catch most features and otherwise create small custom rules to forbid certain syntax.
  • Third-party parsers could easily pass whatever options they wanted through configuration without us adding a special case.

Implications

  • We'd remove context.ecmaFeatures in favor of context.parserOptions.
  • Rules checking for context.ecmaFeatures.modules should check the sourceType property of the Program node instead.
@ilyavolodin

This comment has been minimized.

Member

ilyavolodin commented Dec 8, 2015

Sounds reasonable. My only small concern is once we implement ecmaVersion we'll start getting requests for es5 and es3 support.

@nzakas

This comment has been minimized.

Member

nzakas commented Dec 8, 2015

Acorn (and therefore Espree) supports ecmaVersion 3-6, so that's not a problem.

@ilyavolodin

This comment has been minimized.

Member

ilyavolodin commented Dec 8, 2015

In that case, I have no objections.

@hzoo

This comment has been minimized.

Member

hzoo commented Dec 9, 2015

Sounds good - especially if a lot of people are using es6 already and others are confused about the other options.

@xjamundx

This comment has been minimized.

Contributor

xjamundx commented Dec 9, 2015

I generally am in favor of this idea. 👍

Here are a few things I'd like to discuss:

  • ecmaVersion really bugs me and is super imprecise with es2015/16/etc, but I agree for consistency it's probably fine
  • ecmaFeatures for the remaining stuff makes sense
  • We should also look at https://github.com/babel/babel/tree/master/packages/babylon which I suspect will be a super important parser moving forward. Their version of our ecmaFeatures is plugins and they have es6 by default....so actually yeah probably don't copy them :)

I think this proposal pretty well matches the usage patterns of a lot of users and certainly will work well for us at paypal.

@hzoo

This comment has been minimized.

Member

hzoo commented Dec 9, 2015

Could change it to be es2015 instead of es6 if we wanted.

@btmills

This comment has been minimized.

Member

btmills commented Dec 9, 2015

I like it. 👍

I think ecmaVersion as 5/6/7 is fine, but if we get requests to support 201x, that implementation is simple:

parserOptions.ecmaVersion %= 2010
@platinumazure

This comment has been minimized.

Member

platinumazure commented Dec 9, 2015

👍 from me. Is this something we would try to fit into the ESLint 2.0 roadmap, or will it be 3.0 or later?

@nzakas

This comment has been minimized.

Member

nzakas commented Dec 9, 2015

I'd like to get it into 2.0.0 because it will help with flexibility for external parsers.

@nzakas nzakas added accepted and removed evaluating labels Dec 9, 2015

@nzakas nzakas added this to the v2.0.0 milestone Dec 9, 2015

@nzakas nzakas self-assigned this Dec 9, 2015

@pedrottimark

This comment has been minimized.

Member

pedrottimark commented Dec 9, 2015

Your direction is consistent with principle of analogy for learning and usability: make complexity of ESLint config similar to corresponding Babel config. The most recent standard ES version is what many beginning to intermediate devs will need.

Some extra effort to add certain future features or omit certain current features (in ESLint, or in Babel not transpile features after supported in browser or Node) for advanced dev ops is what Alan Cooper calls the principle of commensurate effort.

By the way, if the two projects ever find they are diverging in the amount of effort to configure for common use cases, it seems like a healthy response it too reach out and think it through together :)

@nzakas

This comment has been minimized.

Member

nzakas commented Dec 9, 2015

Changes to Espree: eslint/espree#223

@nzakas

This comment has been minimized.

Member

nzakas commented Dec 9, 2015

Starting work on this.

@ariya

This comment has been minimized.

ariya commented Dec 9, 2015

If I may suggest something, call it languageVersion or something similar.

"Ecma" is a big umbrella (akin to IEEE, ITU, or W3C), rather than just ECMA-262.

@nzakas

This comment has been minimized.

Member

nzakas commented Dec 9, 2015

The convention of ecmaVersion has already been set by other utilities (like Acorn and escope). I'd rather follow the convention than create something that has the same meaning with a different name.

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

@gyandeeps

This comment has been minimized.

Member

gyandeeps commented Dec 10, 2015

I am not in this boat but something to think about: Based on nodejs 4.x, it doesnt support all the ES6 feature by default, now with that being said people might want to turnoff those ES6 feature for their code. How do we tackle that? Or we dont have to tackle that?

Maybe the answer is to not use ESLINT 2.x but then we dont back port bug fixes, so indirectly we are forcing people to move to 2.x.

Again just a thought, I am not opposed to what we are doing right now as I dont have this issue.

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

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

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

@nzakas

This comment has been minimized.

Member

nzakas commented Dec 10, 2015

@gyandeeps as I mentioned in the original description, people can still use no-restricted-syntax to block a lot of features if they so desire. For any others, they can easily write custom rules.

@nzakas nzakas closed this in 1e60065 Dec 11, 2015

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

Merge pull request #4664 from eslint/issue4641
Breaking: Implement parserOptions (fixes #4641)
@domenic

This comment has been minimized.

domenic commented Dec 25, 2015

I think this is really disappointing because it is perpetuating the fiction that is spec versioning throughout the tooling ecosystem. The year in which a feature is ratified by the European Computer Manufacturer's Association should have no bearing on how people view it or use it in their projects.

Features should be considered independently of whatever official "spec version" they are bundled in; more relevant is whether they are supported in your target environment (e.g. Node, Chrome 49, Edge 12 vs. Edge 13 vs. IE11). Spec versions are a fiction, and have become as irrelevant to JS as they are to the rest of the web.

I recognize there are workarounds, and I fully intend to use them. But I'd urge you to reconsider the technical and philosophical implications of a change like this, which bundles features together by their absolutely least important aspect, for no good reason I can understand.

@mysticatea

This comment has been minimized.

Member

mysticatea commented Dec 27, 2015

I have a plan to create a rule to restrict features to the specified target environment of Node.js (mysticatea/eslint-plugin-node#5)

It might be good that a similar rule (more commonly) is created in core.

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