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

Update: ecmaVersion defaults to 5, and allows "latest" #14622

Merged
merged 2 commits into from Jun 12, 2021

Conversation

aladdin-add
Copy link
Member

@aladdin-add aladdin-add commented May 25, 2021

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ x] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

  • set ecmaVersion defaults to 5, make it not depend on espree's defaults
  • allow ecmaVersion => "latest"

Is there anything you'd like reviewers to focus on?

all these changes should not affect 3rd-party parsers.

@eslint-github-bot eslint-github-bot bot added the triage An ESLint team member will look at this issue soon label May 25, 2021
@aladdin-add aladdin-add added core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels May 25, 2021
lib/linter/linter.js Outdated Show resolved Hide resolved
@aladdin-add aladdin-add force-pushed the update/ecma-latest branch 3 times, most recently from c7003b9 to 99604d4 Compare May 26, 2021 03:07
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

We also need to update the docs to mention that “latest” is allowed.

lib/linter/linter.js Outdated Show resolved Hide resolved
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM

lib/linter/linter.js Outdated Show resolved Hide resolved
tests/lib/linter/linter.js Outdated Show resolved Hide resolved
lib/linter/linter.js Outdated Show resolved Hide resolved
@aladdin-add aladdin-add changed the title Update: ecmaVersion allows "latest" Breaking: ecmaVersion defaults to 5, and allows "latest" Jun 3, 2021
@aladdin-add aladdin-add added the breaking This change is backwards-incompatible label Jun 3, 2021
@nzakas
Copy link
Member

nzakas commented Jun 4, 2021

There seems to be some confusion about how ecmaVersion currently works and it seems like some folks are assuming everyone understands already. Can someone outline the various scenarios, including when parser is and isn’t present, and what the result is currently? I think that will help provide the missing context to this PR.

@aladdin-add
Copy link
Member Author

aladdin-add commented Jun 4, 2021

well, I will try. the current ecmaVersion normalization is simple:

  • undefined => undefined
  • 2015 => 6

That's say, eslint depends on the parser's defaults option. In the PR, it changed the normalization to:

  • undefined => 5
  • latest => espree.latestEcmaVersion
  • 2015 => 6

it will be a breaking change, when all the following conditions are met:

  • not set ecmaVersion
  • using a 3rd-party parser, but it's default ecmaVersion is not 5.

@ljharb
Copy link
Sponsor Contributor

ljharb commented Jun 4, 2021

@aladdin-add and also, when that parser actually pays attention to the ecmaVersion?

@mdjermanovic
Copy link
Member

ESLint currently doesn't automatically set parserOptions.ecmaVersion if parserOptions.ecmaVersion wasn't specified in user's configuration.

Espree's default is ecmaVersion: 5. Other parsers have different defaults.

Espree is about to change its default ecmaVersion, but we don't want that change to affect ESLint users.

Ideally, ESLint should automatically set parserOptions.ecmaVersion to 5 if parserOptions.ecmaVersion wasn't specified in user's configuration, whenever ESLint is used with Espree but not with another parser (as it would be a breaking change if that parser had a different default).

The problem is: it seems difficult for Linter to know that the parser is Espree when user specifies parser: "espree", because in that case @eslint/eslintrc loads parser and sends the parser object and absolute path to Linter.

@nzakas
Copy link
Member

nzakas commented Jun 5, 2021

Thanks everyone, so I think what we want is this:

ecmaVersion parser result
undefined undefined parser: espree, ecmaVersion: 5
undefined "espree" parser: espree, ecmaVersion: 5
undefined "other-parser" parser: "other-parser", ecmaVersion: undefined
2022 undefined parser: "espree", ecmaVersion: 2022
2022 "espree" parser: "espree", ecmaVersion: 2022
2022 "other-parser" parser: "other-parser", ecmaVersion: 2022

The problem is: it seems difficult for Linter to know that the parser is Espree when user specifies parser: "espree", because in that case @eslint/eslintrc loads parser and sends the parser object and absolute path to Linter.

Can we compare that object to require("espree") to determine this? Or compare the absolute path to require.resolve("espree")?

@aladdin-add
Copy link
Member Author

Can we compare that object to require("espree") to determine this? Or compare the absolute path to require.resolve("espree")?

I just thought a very edge case: someone might want to use a different version of espree.
parser: "./node_modules/espree"

node_modules
├── espree
└── eslint
    └── node_modules
        └── espree

But I think it's fine to not normalize it - espree was used as a 3rd-party parser in this case.

will update as @nzakas suggested (if no objections).

@mdjermanovic
Copy link
Member

Can we compare that object to require("espree") to determine this? Or compare the absolute path to require.resolve("espree")?

Sounds good to me!

I just thought a very edge case: someone might want to use a different version of espree.
parser: "./node_modules/espree"

node_modules
├── espree
└── eslint
    └── node_modules
        └── espree

But I think it's fine to not normalize it - espree was used as a 3rd-party parser in this case.

I agree. If someone explicitly installs another version of espree, then it's like they're using a third-party parser with its own defaults.

Though this might also happen unintentionally when another dependency has espree as a dependency, and the other version of espree ends up in the top node_modules. But that setup seems already broken. Looks like it generally isn't a good idea to explicitly set "parser": "espree" in eslint config unless the user really wants a different version, because "parser": "espree" doesn't guarantee that the espree version specified in eslint's package.json will be used.

@aladdin-add aladdin-add removed the breaking This change is backwards-incompatible label Jun 6, 2021
@nzakas
Copy link
Member

nzakas commented Jun 7, 2021

I had a realization: Linter is not supposed to use Node.js-specific functionality because it is also used in the browser, so I don't think using require() make much sense.

So, at this point, I think the path of least resistance is to leave the Espree default ecmaVersion as 5. We can still support "latest" in ESLint but don't have to worry about the additional complexity we're running into now. With flat config, we will have an opportunity to choose a different default ecmaVersion for ESLint and it will be much easier to special-case Espree because Linter will have a direct reference to it.

Thoughts>

@aladdin-add
Copy link
Member Author

I had a realization: Linter is not supposed to use Node.js-specific functionality because it is also used in the browser, so I don't think using require() make much sense.

do you mean using require.resolve()? But in the PR, I didn't use it, I just used parser === espree to check it, so it seems not an issue.

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

Ah ok. LGTM. 👍

Copy link
Member

@mdjermanovic mdjermanovic 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!

@nzakas
Copy link
Member

nzakas commented Jun 11, 2021

I just realized that this can be included before v8, do we want to merge now?

@mdjermanovic
Copy link
Member

I just realized that this can be included before v8, do we want to merge now?

Yes, I also think this doesn't depend on eslint/espree@b068cea. We can merge this for an ESLint v7 minor version.

@aladdin-add
Copy link
Member Author

yes, and eslint/espree@b068cea depends on this one(set ecmaVersion defaults to 5) :)

@nzakas nzakas merged commit 831f6b3 into eslint:master Jun 12, 2021
@aladdin-add aladdin-add deleted the update/ecma-latest branch June 12, 2021 01:16
aladdin-add added a commit to aladdin-add/eslint that referenced this pull request Jun 16, 2021
aladdin-add added a commit that referenced this pull request Jun 18, 2021
aladdin-add added a commit to aladdin-add/eslint that referenced this pull request Jun 18, 2021
aladdin-add added a commit to aladdin-add/eslint that referenced this pull request Jun 24, 2021
mdjermanovic pushed a commit that referenced this pull request Jun 26, 2021
* Revert "Revert "Update: ecmaVersion defaults to 5, and allows "latest" (#14622)" (#14711)"

This reverts commit 97d9bd2.

* chore: use parser.$parser to check if it's espree

* chore: add some tests

* chore: not set default 5

* chore: make the $parser non-enumerable

* chore: use symbol

* chore: a small refactor
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Dec 10, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Dec 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants