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

Breaking: validate parser options (fixes #384) #412

Merged
merged 9 commits into from
Apr 1, 2019

Conversation

aladdin-add
Copy link
Member

@aladdin-add aladdin-add commented Mar 14, 2019

it will throw an error if any of the conditions is true:

  • ecmaVersion is invalid
  • sourceType is invalid
  • ecmaVersion < 6 & sourceType: "module"

is there some more conditions need to be covered?

it will throw an error if any of the conditions is true:

+ ecmaVersion is invalid
+ sourceType is invalid
+ ecmaVersion < 6 & sourceType: "module"
Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving some small suggestions. This will be useful to pull into ESLint 6.x-- thanks for doing this!

lib/espree.js Outdated Show resolved Hide resolved
tests/lib/ecma-version.js Outdated Show resolved Hide resolved
platinumazure and others added 3 commits March 14, 2019 22:51
Co-Authored-By: aladdin-add <weiran.zsd@outlook.com>
Co-Authored-By: aladdin-add <weiran.zsd@outlook.com>
Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Copy link
Member

@mysticatea mysticatea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

Copy link
Member

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This mostly looks good, I just have a suggestion for a better error message.

lib/espree.js Outdated Show resolved Hide resolved
lib/espree.js Outdated Show resolved Hide resolved
@not-an-aardvark
Copy link
Member

Does this also fix eslint/eslint#9687, or are more changes in ESLint needed to fix that?

not-an-aardvark and others added 2 commits March 15, 2019 19:13
Co-Authored-By: aladdin-add <weiran.zsd@outlook.com>
Co-Authored-By: aladdin-add <weiran.zsd@outlook.com>
@aladdin-add
Copy link
Member Author

Does this also fix eslint/eslint#9687, or are more changes in ESLint needed to fix that?

we need to upgrade espree, since this will be in a major release. not sure some more, like adding a test?

Copy link
Member

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@simonbrunel
Copy link

So does that mean ESLint shouldn't be used in projects targeting ecmaVersion < 2015, while not using any transpiler but for which sources rely on ES6 import/export bundler (e.g. rollup). I don't think ESLint should enforce the ecmaVersion >= 2015 when sourceType: module since this should be an acceptable use case:

module.exports = {
    parserOptions: {
        ecmaVersion: 5,       // disable ES6 features (const, let, etc.)
        sourceType: module    // allow ESM import/export syntax
    }
}

@not-an-aardvark
Copy link
Member

@simonbrunel To be clear, ESLint already does not support that case. The config you showed currently has the effect of setting ecmaVersion: 2015 despite the ecmaVersion: 5 value provided in the config. This change just makes the error more obvious to avoid setting any misleading expectations.

@simonbrunel
Copy link

@not-an-aardvark yes I know, but these kind of projects exist and I don't think ESLint should exclude them.

@simonbrunel
Copy link

What I mean is instead of setting ecmaVersion to 2015 automatically or trigger an error if it doesn't match when sourceType: module, it may be better to make a distinction between the scope of these 2 options and allow projects that use ESM import/export to disable other ES6 features.

@aladdin-add
Copy link
Member Author

you can use eslint-plugin-es.

@simonbrunel
Copy link

@aladdin-add thanks, I will give it a try!

@not-an-aardvark not-an-aardvark merged commit 493d464 into eslint:master Apr 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants