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.

Sign up for GitHub

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

Jump to bottom

Docs: deprecate experimentalObjectRestSpread #9986

Merged
merged 3 commits into from Feb 19, 2018
Merged

Docs: deprecate experimentalObjectRestSpread #9986

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
63d3df5
Docs: deprecate experimentalObjectRestSpread
mysticatea Feb 17, 2018
1edd701
reword
mysticatea Feb 19, 2018
233699f
add the
mysticatea Feb 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion docs/user-guide/configuring.md
View file
Open in desktop
Expand Up @@ -32,7 +32,6 @@ Parser options are set in your `.eslintrc.*` file by using the `parserOptions` p
* `globalReturn` - allow `return` statements in the global scope
* `impliedStrict` - enable global [strict mode](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode) (if `ecmaVersion` is 5 or greater)
* `jsx` - enable [JSX](https://facebook.github.io/jsx/)
* `experimentalObjectRestSpread` - enable support for the experimental [object rest/spread properties](https://github.com/tc39/proposal-object-rest-spread) (**IMPORTANT:** This is an experimental feature that may change significantly in the future. It's recommended that you do *not* write rules relying on this functionality unless you are willing to incur maintenance cost when it changes.)

Here's an example `.eslintrc.json` file:

Expand All @@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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!

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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


## Specifying Parser

By default, ESLint uses [Espree](https://github.com/eslint/espree) as its parser. You can optionally specify that a different parser should be used in your configuration file so long as the parser meets the following requirements:
Expand Down