Consolidate contiguous var declarations in destructuring transform

[motiz88]

Q	A
Bug fix?	yes
Breaking change?	no
New feature?	no
Deprecations?	no
Spec compliancy?	no
Tests added/pass?	yes
Fixed tickets	#3081
License	MIT

Fixes #3081 by never trying to inject multiple VariableDeclaration nodes into a for loop's init clause - we now always deduplicate them to a single node because they will all have the same kind. The order is maintained so execution order of any init expressions is the same.

This solution has the stylistic side effect of reducing the number of vars in the output, and neatly mapping each input declaration to one grouped output declaration.

I purposely didn't touch the core destructuring logic in any way and implemented this as a second pass over the generated nodes array. If anyone prefers a different implementation, let me know!

[codecov-io]

Current coverage is 88.82% (diff: 100%)

Merging #4690 into master will decrease coverage by <.01%

@@             master      #4690   diff @@
==========================================
  Files           195        195          
  Lines         13794      13812    +18   
  Methods        1427       1427          
  Messages          0          0          
  Branches       3176       3181     +5   
==========================================
+ Hits          12254      12269    +15   
- Misses         1540       1543     +3   
  Partials          0          0          

Powered by Codecov. Last update 33eb56a...46087c6

[hzoo]

Nice work, looks good!

should we early return on !nodes.length?

[motiz88]

I don't think that would ever be hit. Actually, I think the nodesOut.length !== 1 branch is dead code now, which means the new loop can almost definitely be folded into the one above it somehow and nodes replaced with a scalar; I'm just not sure how the upper loop works exactly (what does t.inherits do? etc)

[hzoo]

I'm not sure yet either - we can do a fix in another PR as followup though?

[danez]

Isn't tail.kind === node.kind always true. Which then also leads to nodesOut.length always having one element as already mentioned?

[hzoo]

Right I think that's what @motiz88 was getting at in the comment ^

[danez]

I think we should clean it up before merging, so we don't forget it later :)

[motiz88]

In light of my recent findings when trying to simplify this - I've had to make this even more verbose in 46087c6 as it is actually not correct to assume that either tail or node are VariableDeclarations, nor that they have the same kind.

[motiz88]

On it.

On Oct 12, 2016 11:33, "Daniel Tschinder" notifications@github.com wrote:

@danez commented on this pull request.

In packages/babel-plugin-transform-es2015-destructuring/src/index.js
#4690:

@@ -492,7 +492,21 @@ export default function ({ types: t }) {
}
}

•    path.replaceWithMultiple(nodes);
  

•    const nodesOut = [];
  

•    for (const node of nodes) {
  

•      const tail = nodesOut[nodesOut.length - 1];
  

•      if (tail && tail.kind === node.kind) {
  

I think we should clean it up before merging, so we don't forget it later
:)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#4690, or mute the thread
https://github.com/notifications/unsubscribe-auth/ACJHpQ340hCRDT0SZfFmsPfhXagEPsJtks5qzJtigaJpZM4KQJTE
.

[motiz88]

Okay, seems like my assumption that nodes are all VariableDeclarations of the same kind here is false in a number of cases (see test failures for 15cb373). In some places kind has different values; worse, in others we have embedded assignment statements.

So at this point, I think optimizing beyond what we had in 94a6b23 would require some serious (painful) digging around in DestructuringTransformer for no clear benefit. I'm gonna go ahead and revert 15cb373 and we can see what's next from there.