Docs: Add rest and spread operator changes to migration guide #10416

yannickcr · 2018-05-29T13:36:11Z

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)

While updating eslint-plugin-react for the upcoming ESLint v5 I noticed some changes that were not addressed in the current migration guide:

The spread operators now always have the SpreadElement type, they previously had the ExperimentalSpreadProperty type when the experimentalObjectRestSpread was enabled.
The spread operators now always have the RestElement type, they previously had the ExperimentalRestProperty type when the experimentalObjectRestSpread was enabled.

This is due that the backward compatibility is handled by forcing the ecmaVersion in #10230

This change add 2 new migration guides in the "Breaking changes for plugin/custom rule developers" section:

When using the default parser, spread operators now have type SpreadElement
When using the default parser, rest operators now have type RestElement

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

Plugin/custom rule developers should already have handled SpreadElement/RestElement in their plugins, but it is not always the case so I think it's worth it to add this in the migration guide.
As stated at the top of the migration guide "The lists below are ordered roughly by the number of users each change is expected to affect, where the first items are expected to affect the most users." . I'm not sure how many plugin developers will be affected by this change, so in doubt I placed the new guides just bellow a similar change.
English is not my primary language, so any correction is welcome 😉

platinumazure

This generally looks great, thanks for adding this! I've left a few inline comments.

I think these sections are generally in the right place, but I assume plugins that use spread/rest operators are slightly more common than plugins that use JSX. So maybe these sections should be moved to immediately above the JSX section? But that's not critical and shouldn't block this from being merged.

platinumazure · 2018-05-29T13:42:39Z

docs/user-guide/migrating-to-5.0.0.md

@@ -194,6 +196,22 @@ When parsing JSX code like `<a>foo</a>`, the default parser will now give the `f

 **To address:** If you have written a custom rule that relies on text nodes in JSX elements having the `Literal` type, you should update it to also work with nodes that have the `JSXText` type.

+## <a name="spread-operators"></a> When using the default parser, spread operators now have type `SpreadElement`
+
+Previously, when parsing JS code like `const foo = {...data}` with the `experimentalObjectRestSpread` option enabled the default parser would set the `...data` AST node the `ExperimentalSpreadProperty` type.


Could you please add a comma after "with the experimentalObjectRestSpread option enabled" (after the word "enabled")?

And I think the second half of the sentence might read more naturally like this:

the default parser would generate an `ExperimentalSpreadProperty` node type for the `...data` spread element.

(But I may be wrong here.)

platinumazure · 2018-05-29T13:45:22Z

docs/user-guide/migrating-to-5.0.0.md

+
+Previously, when parsing JS code like `const foo = {...data}` with the `experimentalObjectRestSpread` option enabled the default parser would set the `...data` AST node the `ExperimentalSpreadProperty` type.
+
+In ESLint v5, the default parser will now always give the `...data` AST node the `SpreadElement` type, even if the [now deprecated]](#experimental-object-rest-spread) `experimentalObjectRestSpread` option was enabled. This makes the AST compliant with the current ESTree spec.


Should this say:

[[now deprecated] `experimentalObjectRestSpread`](#experimental-object-rest-spread)

Or maybe:

\[now deprecated\] [`experimentalObjectRestSpread`](#experimental-object-rest-spread)

Or for that matter, we could use parentheses to make this simpler.

Also: I think instead of "option was enabled", let's use "option is enabled" (to be consistent with present/future tense of this paragraph).

Oh, I left a spare ]. It was:

[now deprecated](#experimental-object-rest-spread) `experimentalObjectRestSpread`

As you suggest I will use parentheses instead.

platinumazure · 2018-05-29T13:51:11Z

docs/user-guide/migrating-to-5.0.0.md

+
+## <a name="rest-operators"></a> When using the default parser, rest operators now have type `RestElement`
+
+Previously, when parsing JS code like `const {foo, ...rest} = data` with the `experimentalObjectRestSpread` option enabled the default parser would set the `...data` AST node the `ExperimentalRestProperty` type.


Could you please add a comma after "with the experimentalObjectRestSpread option enabled" (after the word "enabled")?

And I think the second half of the sentence might read more naturally like this:

the default parser would generate an `ExperimentalRestProperty` node type for the `...data` rest element.

(But I may be wrong here.)

platinumazure · 2018-05-29T13:51:34Z

docs/user-guide/migrating-to-5.0.0.md

+
+Previously, when parsing JS code like `const {foo, ...rest} = data` with the `experimentalObjectRestSpread` option enabled the default parser would set the `...data` AST node the `ExperimentalRestProperty` type.
+
+In ESLint v5, the default parser will now always give the `...data` AST node the `RestElement` type, even if the [now deprecated]](#experimental-object-rest-spread) `experimentalObjectRestSpread` option was enabled. This makes the AST compliant with the current ESTree spec.


Should this say:

[[now deprecated] `experimentalObjectRestSpread`](#experimental-object-rest-spread)

Or maybe:

\[now deprecated\] [`experimentalObjectRestSpread`](#experimental-object-rest-spread)

Or for that matter, we could use parentheses to make this simpler.

Also: I think instead of "option was enabled", let's use "option is enabled" (to be consistent with present/future tense of this paragraph).

yannickcr · 2018-05-29T20:37:16Z

I made the changes you requested, I hope I did not miss any 😃

platinumazure

LGTM, thanks!

Waiting for another set of eyes before merging.

kaicataldo

LGTM. Thanks for contributing!

platinumazure · 2018-06-01T03:03:16Z

Merged, thanks so much @yannickcr for contributing!

Docs: Add rest and spread operator changes to migration guide

3273002

eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label May 29, 2018

platinumazure added documentation Relates to ESLint's documentation accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels May 29, 2018

platinumazure requested changes May 29, 2018

View reviewed changes

yannickcr added 2 commits May 29, 2018 22:33

Fix review

04f258b

Move rest and spread operator change before JSXText

1480e3f

platinumazure approved these changes May 29, 2018

View reviewed changes

kaicataldo approved these changes Jun 1, 2018

View reviewed changes

not-an-aardvark approved these changes Jun 1, 2018

View reviewed changes

platinumazure merged commit 400d4b5 into eslint:master Jun 1, 2018

yannickcr deleted the rest-spread-migration-guide branch June 2, 2018 12:47

eslint-deprecated bot locked and limited conversation to collaborators Nov 30, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Nov 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Docs: Add rest and spread operator changes to migration guide #10416

Docs: Add rest and spread operator changes to migration guide #10416

yannickcr commented May 29, 2018

platinumazure left a comment

platinumazure May 29, 2018

platinumazure May 29, 2018

yannickcr May 29, 2018

platinumazure May 29, 2018

platinumazure May 29, 2018

yannickcr commented May 29, 2018

platinumazure left a comment

kaicataldo left a comment

platinumazure commented Jun 1, 2018


		Previously, when parsing JS code like `const foo = {...data}` with the `experimentalObjectRestSpread` option enabled the default parser would set the `...data` AST node the `ExperimentalSpreadProperty` type.

		In ESLint v5, the default parser will now always give the `...data` AST node the `SpreadElement` type, even if the [now deprecated]](#experimental-object-rest-spread) `experimentalObjectRestSpread` option was enabled. This makes the AST compliant with the current ESTree spec.


		## <a name="rest-operators"></a> When using the default parser, rest operators now have type `RestElement`

		Previously, when parsing JS code like `const {foo, ...rest} = data` with the `experimentalObjectRestSpread` option enabled the default parser would set the `...data` AST node the `ExperimentalRestProperty` type.

Docs: Add rest and spread operator changes to migration guide #10416

Docs: Add rest and spread operator changes to migration guide #10416

Conversation

yannickcr commented May 29, 2018

platinumazure left a comment

Choose a reason for hiding this comment

platinumazure May 29, 2018

Choose a reason for hiding this comment

platinumazure May 29, 2018

Choose a reason for hiding this comment

yannickcr May 29, 2018

Choose a reason for hiding this comment

platinumazure May 29, 2018

Choose a reason for hiding this comment

platinumazure May 29, 2018

Choose a reason for hiding this comment

yannickcr commented May 29, 2018

platinumazure left a comment

Choose a reason for hiding this comment

kaicataldo left a comment

Choose a reason for hiding this comment

platinumazure commented Jun 1, 2018