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

Docs: deprecate experimentalObjectRestSpread #9986

Merged
merged 3 commits into from Feb 19, 2018

Conversation

@mysticatea
Copy link
Member

mysticatea commented Feb 17, 2018

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

[X] Documentation update

What changes did you make? (Give an overview)

We have released Rest/Spread Properties formally in 4.18.0, so the option experimentalObjectRestSpread has finished its role.

I should make this change in #9943, but I forgot it.

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

Nothing in particular.

@mysticatea mysticatea force-pushed the deprecate-experimental-object-rest-spread branch from de894a0 to 63d3df5 Feb 17, 2018
@@ -53,6 +52,10 @@ Here's an example `.eslintrc.json` file:

Setting parser options helps ESLint determine what is a parsing error. All language options are `false` by default.

### Deprecated

* `ecmaFeatures.experimentalObjectRestSpread` - enable support for the experimental [object rest/spread properties](https://github.com/tc39/proposal-object-rest-spread). This syntax has been supported in `ecmaVersion: 2018`. This option will be removed in ESLint 5.0.0.

This comment has been minimized.

Copy link
@not-an-aardvark

not-an-aardvark Feb 17, 2018

Member

I agree that we should deprecate the option, but I'm not sure it would be a good idea to remove the option in 5.0.0. There are a lot of configs that use it now, so it might be better not to break those configs.

This comment has been minimized.

Copy link
@mysticatea

mysticatea Feb 17, 2018

Author Member

Hm.
I understand your sight, but the migration is not hard. I don't think that we should be afraid the change in a major version up. This removing means we remove experimental node types ExperimentalRestProperty and ExperimentalSpreadProperty. It would be valuable to the plugin ecosystem since we can simplify confusing rest/spread node types.

@eslint/eslint-team What do you think?

This comment has been minimized.

Copy link
@not-an-aardvark

not-an-aardvark Feb 17, 2018

Member

Maybe another solution would be to make the experimentalObjectRestSpread option generate RestElement and SpreadElement nodes rather than ExperimentalRestProperty and ExperimentalSpreadProperty, so that rules would only need to deal with one node type.

This comment has been minimized.

Copy link
@kaicataldo

kaicataldo Feb 17, 2018

Member

I think the least confusing option is to remove it in the next major release. I'm all for removing one-off syntax enabling features in favor of real language versions. However, I'm also slightly concerned with the pain this could cause.

It seems unlikely, but what if someone has their config setup so that they're writing, say, ES6 and are using object rest/spread but don't want to allow other ES6+ syntax (using Babel)? Removing this option would force them to update the ecmaVersion to keep linting their codebase, I think?

Again, seems like an unlikely case. I'm fine with breaking changes, but want to make sure everyone has a clear upgrade path.

These one-off syntax features are proving to be pretty difficult to work with!

This comment has been minimized.

Copy link
@mysticatea

mysticatea Feb 19, 2018

Author Member

Maybe another solution would be to make the experimentalObjectRestSpread option generate RestElement and SpreadElement nodes rather than ExperimentalRestProperty and ExperimentalSpreadProperty, so that rules would only need to deal with one node type.

Indeed, that's a solution. Though I am still thinking better to remove experimentalObjectRestSpread option...

It seems unlikely, but what if someone has their config setup so that they're writing, say, ES6 and are using object rest/spread but don't want to allow other ES6+ syntax (using Babel)? Removing this option would force them to update the ecmaVersion to keep linting their codebase, I think?

We have done similar change in 2.0.0: https://eslint.org/docs/user-guide/migrating-to-2.0.0#language-options. In that case, some users asked a way to disable syntactic features that Node.js doesn't support. Our answer was "node/no-unsupported-feature reports the syntax you cannot use in specific Node version."


Anyway, I believe we should remove experimentalObjectRestSpread as soon as possible because:

  • it's an only exceptional option which opposes our policy. We have stopped flags of each syntactic feature in 2.0.0.
  • it's a troublemaker since we override several acorn's methods to implement the option. The overrides sometimes hid bug fixes in acorn then we had to fix our code as same as acorn. In fact, the implementation of experimentalObjectRestSpread has a bug about duplication check for ES modules. I really want to remove the double management.

This comment has been minimized.

Copy link
@kaicataldo

kaicataldo Feb 19, 2018

Member

Thanks for the in-depth response. I think I'm leaning towards removal in v5.0.0 as well, given the points made by @mysticatea.

This comment has been minimized.

Copy link
@not-an-aardvark

not-an-aardvark Feb 19, 2018

Member

My concern is that there are an extremely large number of users relying on experimentalObjectRestSpread right now, and if we remove it then almost all of those users will get parsing errors after upgrading. To fix the errors will need to figure out what's going on and how to update their configs. It might be particularly confusing for people who are using shareable configs, because they might not even be aware that they were relying on experimental features.

In the past, we've tried to minimize the number of "required config changes" that users need to make in order to upgrade. I think it could cause significant problems for users if we remove the property without having it deprecated for awhile.

This comment has been minimized.

Copy link
@mysticatea

mysticatea Feb 19, 2018

Author Member

OK, I'll open another issue to decide removal timing. This issue focuses on showing the depreciation to users.

Copy link
Member

not-an-aardvark left a comment

LGTM, thanks!

@@ -53,6 +52,10 @@ Here's an example `.eslintrc.json` file:

Setting parser options helps ESLint determine what is a parsing error. All language options are `false` by default.

### Deprecated

* `ecmaFeatures.experimentalObjectRestSpread` - enable support for the experimental [object rest/spread properties](https://github.com/tc39/proposal-object-rest-spread). This syntax has been supported in `ecmaVersion: 2018`. This option will be removed in future.

This comment has been minimized.

Copy link
@not-an-aardvark

not-an-aardvark Feb 19, 2018

Member

Nitpick: I think this should say:

This option will be removed in the future.

@kaicataldo kaicataldo merged commit 7c2cd70 into master Feb 19, 2018
5 checks passed
5 checks passed
commit-message PR title follows commit message guidelines
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor This change is semver-patch
Details
@kaicataldo kaicataldo deleted the deprecate-experimental-object-rest-spread branch Feb 19, 2018
@kaicataldo kaicataldo restored the deprecate-experimental-object-rest-spread branch Feb 19, 2018
@kaicataldo

This comment has been minimized.

Copy link
Member

kaicataldo commented Feb 19, 2018

Whoops, sorry - deleted the branch out of habit.

This was referenced Mar 22, 2018
@renovate renovate bot mentioned this pull request Mar 23, 2018
@eslint eslint bot locked and limited conversation to collaborators Aug 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.