-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Conversation
de894a0
to
63d3df5
Compare
docs/user-guide/configuring.md
Outdated
@@ -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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
docs/user-guide/configuring.md
Outdated
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: I think this should say:
This option will be removed in the future.
Whoops, sorry - deleted the branch out of habit. |
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.