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

Consolidate contiguous var declarations in destructuring transform #4690

Merged
merged 4 commits into from Oct 14, 2016

Conversation

Projects
None yet
4 participants
@motiz88
Contributor

motiz88 commented Oct 6, 2016

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

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Oct 6, 2016

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

codecov-io commented Oct 6, 2016

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

hzoo approved these changes Oct 7, 2016

Nice work, looks good!

should we early return on !nodes.length?

@motiz88

This comment has been minimized.

Show comment
Hide comment
@motiz88

motiz88 Oct 7, 2016

Contributor

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)

Contributor

motiz88 commented Oct 7, 2016

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

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Oct 8, 2016

Member

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

Member

hzoo commented Oct 8, 2016

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

@motiz88

This comment has been minimized.

Show comment
Hide comment
@motiz88

motiz88 Oct 12, 2016

Contributor

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
.

Contributor

motiz88 commented Oct 12, 2016

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

This comment has been minimized.

Show comment
Hide comment
@motiz88

motiz88 Oct 13, 2016

Contributor

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.

Contributor

motiz88 commented Oct 13, 2016

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.

@danez

danez approved these changes Oct 14, 2016

@hzoo hzoo merged commit 9fc51d6 into babel:master Oct 14, 2016

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing 33eb56a...46087c6
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +11.16% compared to 33eb56a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

chrisprice added a commit to chrisprice/babel that referenced this pull request Oct 18, 2016

Consolidate contiguous var declarations in destructuring transform (b…
…abel#4690)

* Consolidate contiguous var declarations in destructuring transform

Fixes babel#3081.

* Simplify var node coalescing in es2015-destructuring

* Revert "Simplify var node coalescing in es2015-destructuring"

This reverts commit 15cb373.

* More careful condition for var coalescing in es2015-destructuring

panagosg7 added a commit to panagosg7/babel that referenced this pull request Jan 17, 2017

Consolidate contiguous var declarations in destructuring transform (b…
…abel#4690)

* Consolidate contiguous var declarations in destructuring transform

Fixes babel#3081.

* Simplify var node coalescing in es2015-destructuring

* Revert "Simplify var node coalescing in es2015-destructuring"

This reverts commit 15cb373.

* More careful condition for var coalescing in es2015-destructuring
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment