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

False negatives with sourceType: module and ecmaVersion <= 5 #9687

Closed
timruffles opened this issue Dec 5, 2017 · 15 comments
Closed

False negatives with sourceType: module and ecmaVersion <= 5 #9687

timruffles opened this issue Dec 5, 2017 · 15 comments

Comments

@timruffles
Copy link

@timruffles timruffles commented Dec 5, 2017

Tell us about your environment

  • ESLint Version:
    v4.12.1
  • Node Version:
    8.4.0
  • npm Version:
    5.3.0

What parser (default, Babel-ESLint, etc.) are you using?
default

Please show your full configuration:

Configuration
module.exports = {
    "parserOptions": {
        "sourceType": "module",
        "ecmaVersion": 5,
    }
}

Running against this file, ecmaVersion is ignored (could also be '3'):

// es6.js
const x = () => `hi`;

What did you expect to happen?

Either, I would see an error like the below, rejecting an unsupported config:

> eslint -c withoutModule.config.js es6.js
Incompatible config: sourceType: module cannot be used with ecmaVersion <= 5

or the (admittedly niche*) use-case of ES6 modules with ES5 syntax would be supported, and I'd see errors:

> eslint -c withoutModule.config.js es6.js
es6.js
  1:1  error  Parsing error: The keyword 'const' is reserved

✖ 1 problem (1 error, 0 warnings)

which I get correctly with this config:

// withoutModule.config.js
module.exports = {
    "parserOptions": {
        "ecmaVersion": 5,
    }
}

*niche: I was using rollup with some old ES5 code, and only wanted to modify the code to add imports. I tried the above, and thought "cool, it's working", but actually it was silently ignoring the ecmaVersion.

What actually happened? Please include the actual, raw output from ESLint.

No errors are reported:

> eslint es6.js
> echo $?
0
@not-an-aardvark
Copy link
Member

@not-an-aardvark not-an-aardvark commented Dec 5, 2017

Thanks for the report. I agree with you that the current interface is a bit confusing. We don't currently support parsing imports without parsing the rest of ES6, and as you've noticed, sourceType: module implicitly enables ES6 parsing.

Having said that, I'm not sure it would be worthwhile to introduce a error for this case, because there are probably a significant number of users relying on this behavior. We try to avoid breaking peoples' builds unnecessarily, even in major releases. (It's probably uncommon for users to do something like { ecmaVersion: 5, sourceType: module } explicitly, but ecmaVersion: 5 is the default, so anyone currently using { sourceType: module } and expecting modules to be parsed is relying on this behavior.)

@timruffles
Copy link
Author

@timruffles timruffles commented Dec 6, 2017

@not-an-aardvark I would say breaking builds here is desirable. If people have this configuration, they're unambiguously asking for ES5 syntax + imports. What they're getting is ES6, with no warning that they're using ES6 syntax. That's dangerous.

@not-an-aardvark
Copy link
Member

@not-an-aardvark not-an-aardvark commented Dec 7, 2017

To clarify: I agree that it would be good to report an error if the user puts { ecmaVersion: 5, sourceType: module } in their configuration. However, I don't think it would be a good idea to report an error if the user only puts { sourceType: module } in their configuration, because the user hasn't explicitly specified ecmaVersion: 5, and in most cases they probably want ES6 if they're using modules.

@timruffles
Copy link
Author

@timruffles timruffles commented Dec 7, 2017

@lapo-luchini
Copy link

@lapo-luchini lapo-luchini commented May 2, 2018

Using WebPack it's possible to use ES6 modules without "really" using them in the output, so checking againt ES5 (or even ES3) would make sense.

@Avaq
Copy link

@Avaq Avaq commented Sep 13, 2018

I would really like to see support for es5 syntax with es6 modules. For me as a library author, this is a very common use-case, and I expect it to remain so until nobody cares about es5 support anymore.

I've been setting things up like this, hoping that support would land "any time now" for almost a year.

My reason for doing so is because it gives me the best of all worlds:

  1. My source is modular and organised.
  2. My source is executable without a compilation step by <script type="module"> and node --experimental-modules.
  3. My source is "tree-shakable" and easily concatenated into a single file by tools such as Rollup
  4. When concatenated, no further compilation is needed to target older environments. This in particular is the accepted standard among front-end developers. Nobody likes to consume libraries that have to be compiled.

@nzakas
Copy link
Member

@nzakas nzakas commented Oct 23, 2018

I think having an error for this use case makes sense, as it's confusing.

TSC Summary: Right now if you use ecmaVersion: 5 with sourceType: module, we silently ignore the ecmaVersion setting and upgrade to ecmaVersion: 6. The end user can't tell that has happened, which may lead to confusion.

TSC Question: Shall we throw an error when ecmaVersion is less than 6 and sourceType: module is used? This would be a breaking change.

@nzakas nzakas self-assigned this Oct 23, 2018
@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Oct 25, 2018

TSC Resolution: This is accepted as a breaking change. Implementation details can be discussed on this issue.

@nzakas nzakas added this to Accepted, ready to implement in v6.0.0 Oct 26, 2018
@platinumazure
Copy link
Member

@platinumazure platinumazure commented Feb 5, 2019

@eslint/eslint-tsc Where do we feel we are at, in terms of the complexity of open questions on this issue? Should this be discussed in an RFC, or do we think we have a generally good idea of the direction here?

@nzakas
Copy link
Member

@nzakas nzakas commented Feb 6, 2019

I think the agreement was to throw an error when sourceType: module and ecmaVersion is less than 6 rather than silently changing it. I'm not sure we need much else to move forward.

aladdin-add added a commit to aladdin-add/eslint that referenced this issue Mar 11, 2019
aladdin-add added a commit to aladdin-add/eslint that referenced this issue Mar 11, 2019
@not-an-aardvark not-an-aardvark moved this from Accepted, ready to implement to Ready to merge in v6.0.0 Mar 15, 2019
@platinumazure
Copy link
Member

@platinumazure platinumazure commented Apr 2, 2019

@not-an-aardvark @aladdin-add Is this resolved by the espree changes?

@not-an-aardvark
Copy link
Member

@not-an-aardvark not-an-aardvark commented Apr 2, 2019

It might be a good idea to do a major (pre)release of espree and update to that version in ESLint before we close this.

not-an-aardvark added a commit that referenced this issue Apr 13, 2019
not-an-aardvark added a commit that referenced this issue Apr 13, 2019
v6.0.0 automation moved this from Ready to merge to Done Apr 13, 2019
not-an-aardvark added a commit that referenced this issue Apr 13, 2019
aladdin-add added a commit to aladdin-add/eslint that referenced this issue Apr 24, 2019
PR eslint#11610 was aimed to fix the issue eslint#9687, however it did not work,
since ecmaVersion has been normalized to 6 (when sourceType is setting to 'module').

this PR removed the normalize behaviour, and fixed failing tests.
aladdin-add added a commit to aladdin-add/eslint that referenced this issue Apr 27, 2019
PR eslint#11610 was aimed to fix the issue eslint#9687, however it did not work,
since ecmaVersion has been normalized to 6 (when sourceType is setting to 'module').

this PR removed the normalize behaviour, and fixed failing tests.
aladdin-add added a commit to aladdin-add/eslint that referenced this issue May 10, 2019
PR eslint#11610 was aimed to fix the issue eslint#9687, however it did not work,
since ecmaVersion has been normalized to 6 (when sourceType is setting to 'module').

this PR removed the normalize behaviour, and fixed failing tests.
not-an-aardvark added a commit that referenced this issue May 11, 2019
…11649)

PR #11610 was aimed to fix the issue #9687, however it did not work,
since ecmaVersion has been normalized to 6 (when sourceType is setting to 'module').

this PR removed the normalize behaviour, and fixed failing tests.
@jefferyto
Copy link

@jefferyto jefferyto commented Jul 4, 2019

I know I'm extremely late to this conversation, but I felt I should mention a use case for es5 syntax checking with es6 modules (in addition to the ones already mentioned):

Rollup uses es6 modules exclusively but doesn't require any other es6 features. If I want to use eslint with Rollup, I am now essentially forced into using es6 validation.

For most forward-looking projects, this perhaps isn't a problem. But if I am writing a library for current/legacy browsers and want to author es5 only, then I only have two options:

  • Just remember to not use es6 features
  • Add babel to transpile any accidental es6 code

Neither of these options appeal to me because I want to author es5 code and want eslint to throw errors when that isn't the case.

@mysticatea
Copy link
Member

@mysticatea mysticatea commented Jul 4, 2019

@jefferyto Previously, ESLint had set ecmaVersion: 6 implicitly automatically if sourceType: "module" was given but ecmaVersion was smaller than 6. This PR just removed that implicit behavior. You have recognized that it had not worked as your expectation now :)

You can use eslint-plugin-es to warn arbitrary syntaxes individually.

@jefferyto
Copy link

@jefferyto jefferyto commented Jul 4, 2019

@mysticatea Thanks for the pointer to eslint-plugin-es - so now I set ecmaVersion: 6, then use the plugin:es/no-2015 preset, then turn off es/no-modules. This is... madness 😂

timvandermeij added a commit to timvandermeij/pdf.js that referenced this issue Aug 24, 2019
This major version bump required two changes:

- The global line in the mobile viewer example should be removed because
  the `.eslintrc` file already defines these globals and with the new
  `eslint` version we otherwise get an error saying "'pdfjsLib' is already
  defined as a built-in global variable".
- The ECMA version for the examples must be set to 6 since we're using
  modules, otherwise we get an error saying "sourceType 'module' is not
  supported when ecmaVersion < 2015". It turns out that the previous
  version of `eslint` already used ECMA version 6 silently even though
  we set 5, see eslint/eslint#9687 (comment),
  so in terms of our code nothing really changes.
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Oct 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
v6.0.0
  
Done
Linked pull requests

Successfully merging a pull request may close this issue.

9 participants