object rest - fix when destructuring in variables/parameters #4755

hzoo · 2016-10-19T14:20:03Z

Description

The issue is that this plugin (http://babeljs.io/docs/plugins/transform-object-rest-spread/) depends on the destructuring/parameter plugins to work correctly. Thus it doesn't work standalone and this PR is trying to fix that!

REPL

Test with http://astexplorer.net/#/Y32VLF7LZ0/16

Test Cases

RestProperty

Parameters

function a({ b, ...c }) {}

VariableDeclaration

const { a, ...b } = c;

ExportNamedDeclaration

export var { a, ...b } = c;

CatchClause

try {} catch ({a, ...b}) {}

AssignmentExpression

({a, ...b} = c);

ForXStatement

for ({a, ...b} of []) {}

SpreadProperty

ObjectExpression

var a = { ...b, ...c }

Testing

Can test locally by replacing in node_modules: https://gist.github.com/hzoo/d269ccfd5758c556c900f47dd752cd61

replace node_modules/babel-plugin-transform-object-rest-spread/lib/index.js

Future

#4755 (comment)

computed keys aren't working but will do that in another pr, ref Correctly support non-string computed keys #3675

Jessidhia · 2016-10-20T01:16:17Z

...ugin-transform-object-rest-spread/test/fixtures/object-rest/variable-destructuring/actual.js

+var { [a]: b, ...c } = z;
+var {x1, ...y1} = z;
+let {x2, y2, ...z2} = z;
+const {w3, x3, y3, ...z4} = z;


Should also check, just to see if things are preserved as expected:

let [ x4, { y4, ...z4 } ] = z let { x: { y: [ y5 ] = [], ...z5 } = {} } = z

Jessidhia · 2016-10-20T01:22:29Z

...in-transform-object-rest-spread/test/fixtures/object-rest/variable-destructuring/expected.js

+var { x1 } = z;
+var y1 = babelHelpers.objectWithoutProperties(z, ["x1"]);
+var { [a]: b } = z;
+var c = babelHelpers.objectWithoutProperties(z, [a]);


(unrelated but) this breaks in real code when typeof a is not string or symbol :(

codecov-io · 2016-11-04T20:49:09Z

Current coverage is 89.32% (diff: 100%)

Merging #4755 into master will increase coverage by 0.04%

@@             master      #4755   diff @@
==========================================
  Files           196        196          
  Lines         13816      13936   +120   
  Methods        1436       1449    +13   
  Messages          0          0          
  Branches       3211       3239    +28   
==========================================
+ Hits          12335      12449   +114   
- Misses         1481       1487     +6   
  Partials          0          0

Powered by Codecov. Last update 62ae3c7...ddfcd88

hzoo · 2016-11-08T22:29:09Z

cc @sebmarkbage

http://astexplorer.net/#/Y32VLF7LZ0/13 based on the destructuring transform there are a few other (weird) ways to use RestProperty?

Common

// VariableDeclaration
const { a, ...b } = c;
// in Parameters
function a({ b, ...c }) {}

// ExportNamedDeclaration
export var { b, ...c } = asdf2;

Odd/not fixed yet

// AssignmentExpression
({a, ...b} = c);

// ForXStatement
for (let {a, ...b} of []) {}

// CatchClause
try {}
catch ({a, ...b}) {}

sebmarkbage · 2016-11-08T23:03:27Z

const { ...a, b } = c; is not valid. Rest is only allowed as the last property.

hzoo · 2016-11-08T23:03:51Z

Oops that is a wrong example! We already fixed those static errors in babylon (https://github.com/babel/babylon/releases/tag/v6.11.3)

DrewML · 2016-11-09T23:00:15Z

Was going to ask if it'd be worth breaking out any of the copied code from visitor/transform code from one of the plugins so both can use it, but in reality that level of abstraction is probably just going to create more of a maintenance pain.

DrewML

Stared at the test cases for quite a bit, and I couldn't find anything you weren't covering.

hzoo · 2016-11-10T01:53:30Z

Yeah exactly - I think in reality not everything is updated/shared that much anyway?

Jessidhia · 2016-11-10T02:09:31Z

...in-transform-object-rest-spread/test/fixtures/object-rest/variable-destructuring/expected.js

+var y1 = babelHelpers.objectWithoutProperties(z, ["x1"]);
+x1++;
+var { [a]: b } = z;
+var c = babelHelpers.objectWithoutProperties(z, [a]);


a is evaluated twice (fine here, but what if it's a CallExpression or a MemberExpression?)

objectWithoutProperties also won't behave correctly if typeof a !== 'string': https://github.com/babel/babel/blob/master/packages/babel-helpers/src/helpers.js#L434

danez · 2016-11-13T08:41:58Z

packages/babel-plugin-transform-object-rest-spread/src/index.js

+  function hasRestProperty(node) {
+    for (let property of (node.properties)) {
+      if (t.isRestProperty(property)) {
+        return true;


to be more type consistent I would also add a return false at the end of the function.

danez · 2016-11-13T08:52:16Z

packages/babel-plugin-transform-object-rest-spread/src/index.js

+          RestProperty(path) {
+            if (!this.originalPath.node) {
+              return;
+            }


Too bad we don't have working coverage reports, would be interested if this is covered. Don't see why it should be falsey.

I think it was because at the time I was getting an infinite loop or the node was removed and I was trying to use it - will remove.

hzoo · 2016-11-15T16:31:35Z

Ok let's do this. 🎉 finally

danez · 2016-11-16T08:44:15Z

👍

gustavohenke · 2016-11-28T13:43:14Z

Docs are still mentioning this PR: https://babeljs.io/docs/plugins/transform-object-rest-spread/

xpepermint · 2016-11-28T13:51:06Z

+ if I include this plugin, webpack ignores other plugins and my file grows from ~80KB to 800KB

Sync this README with https://github.com/babel/babel.github.io/blob/master/docs/plugins/transform-object-rest-spread.md I removed the quote which mention babel#4755 because it's already fixed.

https://babeljs.io/docs/plugins/transform-object-rest-spread/ Object Rest currently depends on the destructuring transform and parameters transform Even if you are using Node 6 or a platform that supports destructuring, transform-es2015-destructuring and transform-es2015-parameters will currently need to be enabled if using object rest properties. There is a PR open to fix this and make this transform standalone: babel/babel#4755

swernerx · 2016-12-06T06:27:28Z

It seems like the docs here are still wrong about this: https://babeljs.io/docs/plugins/transform-object-rest-spread/

) * object rest - fix when destructuring in variables/parameters * fixes + ExportNamedDeclaration support * Account for CatchClause * support ForXStatement * support assignment expression? + PR fixes

migueloller · 2017-01-19T17:29:49Z

I've been having issues with Babel and transform-object-rest-spread. Ultimately the issue went away after including transform-es2015-destructuring.

When looking at the transpiled code it looks like the line that is causing the error was transpiled away but looking further it seems that it was moved elsewhere.

Here's what my .babelrc looks like:

{
  "presets": [
    ["env", {
      "targets": {
        "node": 7.2
      }
    }]
  ],
  "plugins": [
    "transform-class-properties",
    "transform-object-rest-spread",
    "transform-flow-strip-types"
  ]
}

I tried reproducing this in the Babel REPL but was unable to. Is it possible I'm seeing some type of edge case?

hzoo · 2017-01-19T19:25:51Z

In order to help you more you'll need to provide a repro case with code, otherwise it's not easy to find the issue. There are a few issues with it already that have PRs. #5088, #5151.

if you need more want help, I would recommend checking out our slack community

migueloller · 2017-01-19T19:47:53Z

I think #5151 is the problem I'm seeing! I'll wait for it to be merged and check back again because I can't find a way to reproduce my specific case.

Thanks @hzoo!

olalonde · 2017-01-19T19:57:37Z

On a related note, would be nice to have a tool that takes a git commit as input and tells you whether it was published on npm (bonus: alerts you by email when it is :)).

hzoo · 2017-01-19T19:58:16Z

😄 yeah I want the bot (babel-bot) to make a message when the merged pr is released and in what version etc.

edit: https://github.com/babel/babel-bot

hzoo added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Oct 19, 2016

hzoo mentioned this pull request Oct 19, 2016

Object rest spread not working correctly in object destructuring (T7086) #4074

Closed

Jessidhia reviewed Oct 20, 2016

View reviewed changes

jhen0409 mentioned this pull request Oct 25, 2016

[Proposal] Remove ES2015 preset when Node 6 active LTS electron-react-boilerplate/electron-react-boilerplate#275

Closed

2 tasks

hzoo mentioned this pull request Oct 28, 2016

unexpected token (rest spread operator) babel/babel-loader#170

Closed

hzoo force-pushed the object-rest-standalone branch from c5dd9b6 to 7744976 Compare November 4, 2016 20:49

hzoo force-pushed the object-rest-standalone branch from 7744976 to d4f16c7 Compare November 8, 2016 22:25

hzoo force-pushed the object-rest-standalone branch 3 times, most recently from 98a6238 to 7e173dd Compare November 8, 2016 22:49

DrewML approved these changes Nov 9, 2016

View reviewed changes

Jessidhia reviewed Nov 10, 2016

View reviewed changes

danez reviewed Nov 13, 2016

View reviewed changes

hzoo added 5 commits November 14, 2016 15:41

object rest - fix when destructuring in variables/parameters

18827d4

fixes + ExportNamedDeclaration support

6c16c75

Account for CatchClause

738e5bd

support ForXStatement

60fca65

support assignment expression? + PR fixes

ddfcd88

hzoo force-pushed the object-rest-standalone branch from 3af851e to ddfcd88 Compare November 14, 2016 20:42

hzoo mentioned this pull request Nov 15, 2016

Investigate if object rest works correctly in tests on master facebook/create-react-app#904

Closed

hzoo merged commit 5e0508d into master Nov 15, 2016

danez deleted the object-rest-standalone branch November 16, 2016 08:43

valscion mentioned this pull request Nov 17, 2016

Remove unnecessary transform plugins for object spread to work facebook/create-react-app#1052

Merged

hzoo mentioned this pull request Nov 18, 2016

Using object destructuring rest parameters causes default argument to become evaluated before rest parameters are declared #4867

Closed

jprichardson mentioned this pull request Dec 1, 2016

useBuiltIns for transform-object-rest-spread has stopped working for 6.19.0 #4921

Closed

lukyth mentioned this pull request Dec 2, 2016

Update transform-object-rest-spread's README from babel.github.io [skip ci] #4930

Merged

kaicataldo mentioned this pull request Dec 6, 2016

Remove caveat from transform-object-rest-spread.md babel/website#1039

Merged

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 6, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

object rest - fix when destructuring in variables/parameters #4755

object rest - fix when destructuring in variables/parameters #4755

hzoo commented Oct 19, 2016 •

edited

Loading

Jessidhia Oct 20, 2016

Jessidhia Oct 20, 2016

codecov-io commented Nov 4, 2016 •

edited

Loading

hzoo commented Nov 8, 2016 •

edited

Loading

sebmarkbage commented Nov 8, 2016

hzoo commented Nov 8, 2016 •

edited

Loading

DrewML commented Nov 9, 2016

DrewML left a comment

hzoo commented Nov 10, 2016

Jessidhia Nov 10, 2016 •

edited

Loading

Jessidhia Nov 10, 2016

danez Nov 13, 2016

danez Nov 13, 2016

hzoo Nov 14, 2016

hzoo commented Nov 15, 2016

danez commented Nov 16, 2016

gustavohenke commented Nov 28, 2016

xpepermint commented Nov 28, 2016 •

edited

Loading

swernerx commented Dec 6, 2016

migueloller commented Jan 19, 2017

hzoo commented Jan 19, 2017

migueloller commented Jan 19, 2017

olalonde commented Jan 19, 2017

hzoo commented Jan 19, 2017 •

edited

Loading

object rest - fix when destructuring in variables/parameters #4755

object rest - fix when destructuring in variables/parameters #4755

Conversation

hzoo commented Oct 19, 2016 • edited Loading

Description

REPL

Test Cases

RestProperty

SpreadProperty

Testing

Future

Jessidhia Oct 20, 2016

Choose a reason for hiding this comment

Jessidhia Oct 20, 2016

Choose a reason for hiding this comment

codecov-io commented Nov 4, 2016 • edited Loading

Current coverage is 89.32% (diff: 100%)

hzoo commented Nov 8, 2016 • edited Loading

Odd/not fixed yet

sebmarkbage commented Nov 8, 2016

hzoo commented Nov 8, 2016 • edited Loading

DrewML commented Nov 9, 2016

DrewML left a comment

Choose a reason for hiding this comment

hzoo commented Nov 10, 2016

Jessidhia Nov 10, 2016 • edited Loading

Choose a reason for hiding this comment

Jessidhia Nov 10, 2016

Choose a reason for hiding this comment

danez Nov 13, 2016

Choose a reason for hiding this comment

danez Nov 13, 2016

Choose a reason for hiding this comment

hzoo Nov 14, 2016

Choose a reason for hiding this comment

hzoo commented Nov 15, 2016

danez commented Nov 16, 2016

gustavohenke commented Nov 28, 2016

xpepermint commented Nov 28, 2016 • edited Loading

swernerx commented Dec 6, 2016

migueloller commented Jan 19, 2017

hzoo commented Jan 19, 2017

migueloller commented Jan 19, 2017

olalonde commented Jan 19, 2017

hzoo commented Jan 19, 2017 • edited Loading

hzoo commented Oct 19, 2016 •

edited

Loading

codecov-io commented Nov 4, 2016 •

edited

Loading

hzoo commented Nov 8, 2016 •

edited

Loading

hzoo commented Nov 8, 2016 •

edited

Loading

Jessidhia Nov 10, 2016 •

edited

Loading

xpepermint commented Nov 28, 2016 •

edited

Loading

hzoo commented Jan 19, 2017 •

edited

Loading