Skip to content
This repository has been archived by the owner on Aug 18, 2021. It is now read-only.

RFC: Do not set a default ecmaVersion or sourceType #458

Closed
wants to merge 1 commit into from

Conversation

zertosh
Copy link
Member

@zertosh zertosh commented Mar 26, 2017

This is a super breaking change, but it ensures correctness in ESLint's rules, and eliminates surprises.

Before this change, babel-eslint set defaults for sourceType and ecmaVersion that it applied to Babylon and escope. However, these values could not propagate to the ESLint rules themselves or ESLint APIs. This resulted in incorrect behavior by certain rules. Consider these examples:

Making things even more surprising is the fact that setting env: es6 sets the ecmaVersion to 6 but doesn't change sourceType (https://github.com/eslint/eslint/blob/6a718ba/conf/environments.js#L98-L103).

@hzoo
Copy link
Member

hzoo commented Jun 8, 2017

I guess we should do this to keep consistent behavior in ESLint, would be breaking but we are doing a v7 atm.

`allowImportExportEverywhere` can be set to true to allow import and export declarations to appear anywhere a statement is allowed if your build environment supports that. By default, import and export declarations can only appear at a program's top level.
`codeFrame` can be set to false to disable the code frame in the reporter. This is useful since some eslint formatters don't play well with it.
* `sourceType`, `ecmaVersion`, `ecmaFeatures.globalReturn` and `ecmaFeatures.impliedStrict` should be set in accordance with http://eslint.org/docs/user-guide/configuring#specifying-parser-options.
- `ecmaVersion` is disregarded by `babel-eslint`, however, some ESlint rules depend on this value being set correctly.
Copy link
Member

Choose a reason for hiding this comment

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

What would it take to make the first part not true?

`codeFrame` can be set to false to disable the code frame in the reporter. This is useful since some eslint formatters don't play well with it.
* `sourceType`, `ecmaVersion`, `ecmaFeatures.globalReturn` and `ecmaFeatures.impliedStrict` should be set in accordance with http://eslint.org/docs/user-guide/configuring#specifying-parser-options.
- `ecmaVersion` is disregarded by `babel-eslint`, however, some ESlint rules depend on this value being set correctly.
- If `sourceType` is not set to `"module"`, then using `import`/`export` will error.
Copy link
Member

Choose a reason for hiding this comment

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

Also, files will not automatically be in strict mode.

@hzoo
Copy link
Member

hzoo commented Jun 8, 2017

I guess the reason we had the defaults earlier though is that babylon automatically parses what is spec so the ecmaversion should at least be es2017 atm? and we assume modules normally

@kaicataldo
Copy link
Member

kaicataldo commented Jun 8, 2017

Unless I'm missing something, seems to me we shouldn't remove the defaults. I agree with @hzoo - I think default for ecmaVersion should be the latest version available in ESLint and we should leave the default for sourceType as well. So to me it seems like the changes we might want are to update the ecmaVersion default to 2017 and then add documentation saying that the config should also include the following to make sure that ESLint core is doing the right thing:

"parserOptions": {
    "ecmaVersion": 2017
},

Edit: Change 8 to 2017 for the sake of clarity in response to @ljharb's comment below

@ljharb
Copy link
Member

ljharb commented Jun 8, 2017

Please let's make sure documentation uses year, not edition number - it was ES6/ES2015, but nobody calls it ES8 - it's ES2017.

@danez
Copy link
Member

danez commented Jun 15, 2017

Related to this: I created this ticket eslint/eslint#8744 which is the opposite to this PR. :)
Basically what I thought would be nice is if the parser could decide what the default options are in a way that eslint understands it and uses them everywhere correctly. See issue for details.

@kaicataldo
Copy link
Member

@zertosh Is this still something you'd like to advocate for?

@zertosh
Copy link
Member Author

zertosh commented Jan 23, 2019

@kaicataldo, I'd love to, but I just don't have time these days :(

@kaicataldo
Copy link
Member

Okay! Will close this, but happy to have this discussion again in the future!

@kaicataldo kaicataldo closed this Jan 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants