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

Proposal: removing parserOptions.experimentalObjectRestSpread option #9990

Closed
mysticatea opened this issue Feb 19, 2018 · 11 comments
Closed

Proposal: removing parserOptions.experimentalObjectRestSpread option #9990

mysticatea opened this issue Feb 19, 2018 · 11 comments

Comments

@mysticatea
Copy link
Member

@mysticatea mysticatea commented Feb 19, 2018

Context

Since ESLint 4.18.0, we have supported Rest/Spread Properties in formal with parserOptions.ecmaVersion: 2018. So the parserOptions.experimentalObjectRestSpread option has finished its role.

The parserOptions.experimentalObjectRestSpread option has some confusion because we added it early time.

  • There are three node types for the same syntax; ExperimentalRestProperty, RestProperty, and RestElement. And spread properties are so. This is unfortunate for plugin ecosystem.

Also, the parserOptions.experimentalObjectRestSpread option sometimes has hidden bug fixes in acorn. For example:

The cause is that we make some overridden methods on espree to implement it. In fact, the implementation of parserOptions.experimentalObjectRestSpread has a bug about the duplication check of ES modules currently.

export const b = 1
export const {a, ...b} = obj // "SyntaxError: Identifier 'b' has already been declared" in `ecmaVersion:2018`, but not in `experimentalObjectRestSpread:true`.

Proposal

Remove the parserOptions.experimentalObjectRestSpread option in the next major version, 5.0.0.
To replace it by parserOptions.ecmaVersion: 2018 to migrate.

This is similar to Migrating to 2.0.0 we have done in past.

Pros

  • Reducing confusion about AST node types. This removal removes ExperimentalRestProperty and ExperimentalSpreadProperty.
  • Reducing bugs. This removal removes the double management of the implementation of Rest/Spread Properties. This will prevent bugs such as eslint/espree#341 and eslint/espree#353. We can get bug fixes in acorn quickly in the future.
  • Making consistent. We specify ECMA262 Standard version by parserOptions.ecmaVersion option since 2.0.0. The parserOptions.experimentalObjectRestSpread option is the last exception.

Cons

  • Many users are using the parserOptions.experimentalObjectRestSpread option. This removal can cause confusion in migrating to 5.0.0.
@platinumazure
Copy link
Member

@platinumazure platinumazure commented Feb 26, 2018

My thoughts (off the cuff): It's hard to say how many people are relying on this, so I wonder if we should consider deferring this for 5.0 and implementing in 6.0, to allow more time for rule/plugin authors to make the transition.

@not-an-aardvark
Copy link
Member

@not-an-aardvark not-an-aardvark commented Mar 1, 2018

In today's TSC meeting, the TSC decided to do the following:

  • Remove the experimentalObjectRestSpread option from espree in a breaking change.
  • Update eslint@5 to convert experimentalObjectRestSpread: true to ecmaVersion: 2018 and print a warning message.
  • Plan to remove this behavior in eslint@6.
@not-an-aardvark not-an-aardvark moved this from Needs discussion to Accepted, ready to implement in v5.0.0 Mar 1, 2018
@ljharb
Copy link
Contributor

@ljharb ljharb commented Mar 1, 2018

Could it be kept, but only for ecmaVersions 2015-2017?

@not-an-aardvark
Copy link
Member

@not-an-aardvark not-an-aardvark commented Mar 1, 2018

My impression is that the team is generally not in favor of keeping the option, for a few reasons:

  • The option adds significant complexity to espree.
  • The option was experimental and was never intended to be a long-term feature.
  • In general, we don't have individual flags for syntax features.
  • The option adds significant complexity for rule authors, because there are several different AST representations for object rest/spread emitted by different parsers. Changing to the ecmaVersion: 2018 behavior would simplify the situation.

For further discussion, see #9986 (comment).

@ljharb
Copy link
Contributor

@ljharb ljharb commented Mar 1, 2018

My concern is that there may be codebases that wish to keep object rest/spread but do not wish to enable async/await, or async iterators, or **.

@not-an-aardvark
Copy link
Member

@not-an-aardvark not-an-aardvark commented Mar 1, 2018

The recommendation for those codebases would be to enable ecmaVersion: 2018 and use something like no-restricted-syntax to forbid syntax that they don't want to allow. (Alternatively, they could use a different parser, but that would probably be harder.) We generally don't support toggling syntax features independently in the parser -- experimentalObjectRestSpread was a temporary exception.

not-an-aardvark added a commit to eslint/espree that referenced this issue Mar 29, 2018
Users who want to use object rest/spread should enable `ecmaVersion: 2018` instead. As discussed in eslint/eslint#9990, ESLint 5 will translate the `experimentalObjectRestSpread` option to `ecmaVersion: 2018` and emit a deprecation warning.
aladdin-add added a commit to eslint/espree that referenced this issue Mar 30, 2018
* Breaking: remove experimentalObjectRestSpread option

Users who want to use object rest/spread should enable `ecmaVersion: 2018` instead. As discussed in eslint/eslint#9990, ESLint 5 will translate the `experimentalObjectRestSpread` option to `ecmaVersion: 2018` and emit a deprecation warning.

* Remove experimentalObjectRestSpread from docs
btmills added a commit that referenced this issue Mar 30, 2018
This upgrades Espree to v4.0.0-alpha.0 and removes our duplicated tests
that pair `experimentalObjectRestSpread` and `ecmaVersion: 2018`. It
does not implement the deprecation warning or translating the former to
the latter in the background.
btmills added a commit that referenced this issue Mar 30, 2018
This upgrades Espree to v4.0.0-alpha.0 and removes our duplicated tests
that pair `experimentalObjectRestSpread` and `ecmaVersion: 2018`. It
does not implement the deprecation warning or translating the former to
the latter in the background.
@btmills
Copy link
Member

@btmills btmills commented Mar 30, 2018

As suggested by @not-an-aardvark:

As a separate change later on, we can probably remove all of the rule code that tests for ExperimentalRestProperty etc.

btmills added a commit that referenced this issue Mar 30, 2018
This upgrades Espree to v4.0.0-alpha.0 and removes our duplicated tests
that pair `experimentalObjectRestSpread` and `ecmaVersion: 2018`. It
does not implement the deprecation warning or translating the former to
the latter in the background.
@mysticatea
Copy link
Member Author

@mysticatea mysticatea commented Apr 2, 2018

FWIW, I have published a plugin to disallow each syntactic feature individually: https://www.npmjs.com/package/eslint-plugin-es

@mysticatea
Copy link
Member Author

@mysticatea mysticatea commented Apr 16, 2018

I'll work on this.

mysticatea added a commit that referenced this issue Apr 16, 2018
And it generates a deprecation warning to show that the option has been
deprecated.
@mysticatea mysticatea moved this from Accepted, ready to implement to Ready to merge in v5.0.0 Apr 16, 2018
@satazor
Copy link
Contributor

@satazor satazor commented Apr 22, 2018

I share the same concerns as @ljharb.

Lets consider a project that was written with Node.js >= 8.6 in mind. According to node.green, only the object spread and object rest spread features from es2018 are implemented. Therefore, it would be safer to declare ecmaVersion: 2017 and enable experimentalObjectRestSpread in said projects. If didn't do so, using for await of would not cause any linting errors, but would fail when running the code.

This was just an example. There are a lot of other projects that are locked to ecmaVersion < 2018 but use object rest/spread because they either use Node.js >= 8.6 or use Babel.

Please consider keeping experimentalObjectRestSpread for ecmaVersion < 2018.

v5.0.0 automation moved this from Ready to merge to Done Apr 28, 2018
not-an-aardvark added a commit that referenced this issue Apr 28, 2018
* Update: re-enable experimentalObjectRestSpread (fixes #9990)

And it generates a deprecation warning to show that the option has been
deprecated.

* move warning to config-validator

* revert some unnecessary changes

* move ecmaVersion=9 to resolveParserOptions function

* change error code of deprecation warnings

* make tests robuster

* fix Node 6.x problem.

* fix Node 6.x problem.
@mysticatea
Copy link
Member Author

@mysticatea mysticatea commented Apr 29, 2018

@satazor Please consider to use node/no-unsupported-features rule in eslint-plugin-node

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Oct 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
v5.0.0
  
Done
Linked pull requests

Successfully merging a pull request may close this issue.

6 participants