Skip to content

Fix(merge-sibling-var): force recalc ref when concatenating for-loop vars (#485)#713

Merged
boopathi merged 3 commits intobabel:masterfrom
garyyeap:fix-for-loop-concat
Nov 12, 2017
Merged

Fix(merge-sibling-var): force recalc ref when concatenating for-loop vars (#485)#713
boopathi merged 3 commits intobabel:masterfrom
garyyeap:fix-for-loop-concat

Conversation

@garyyeap
Copy link
Copy Markdown
Contributor

@garyyeap garyyeap commented Nov 3, 2017

Fix #594
Fix #430
Fix #412
Fix #554
Fix #581
Fix #477
Fix #720
Fix #485

@garyyeap garyyeap requested a review from boopathi as a code owner November 3, 2017 15:16
@garyyeap
Copy link
Copy Markdown
Contributor Author

garyyeap commented Nov 3, 2017

Not sure how to write test as some issues only occur when using mergeVars, deadcode or mangle at the same time

@boopathi
Copy link
Copy Markdown
Member

boopathi commented Nov 7, 2017

Thanks a lot. I'll merge it with tests. You can add test cases in babel-preset-minify/__tests__

// references and binding until babel/babel#4818 resolved
path.remove();
init.remove();
next.node.init = t.variableDeclaration("var", declarations);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

next.replaceWith(t.variableDeclaration("var", declarations))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also fix this one? Use babel path API instead of mutating node.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried & failed with this way, the output eliminated the for-loop

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. It's

init.replaceWith(t.variableDeclaration("var", declarations);

and init.remove() should be removed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thanks for correcting

@boopathi boopathi added the Tag: Bug Fix Pull Request fixes a bug label Nov 7, 2017
@boopathi boopathi merged commit 8ef24cb into babel:master Nov 12, 2017
@garyyeap garyyeap changed the title Fix(merge-sibling-var): force recalc ref when concat for-loop var (#485) Fix(merge-sibling-var): force recalc ref when concatenating for-loop var (#485) Nov 13, 2017
@garyyeap garyyeap deleted the fix-for-loop-concat branch November 13, 2017 11:08
@boopathi boopathi added this to the 1.0 milestone Nov 19, 2017
@garyyeap garyyeap changed the title Fix(merge-sibling-var): force recalc ref when concatenating for-loop var (#485) Fix(merge-sibling-var): force recalc ref when concatenating for-loop vars (#485) Nov 23, 2017
@edmorley
Copy link
Copy Markdown

Hi!

This is breaking the build in mozilla/hawk#223 - I don't suppose a point release could be made that includes this fix? :-)

@boopathi
Copy link
Copy Markdown
Member

@edmorley Every commit to master is released as a canary version - docs - https://github.com/babel/minify/tree/master/docs#canary-version ... But it's ONLY for ease of testing. Would not recommend using canary tag committed in your package.json.

@allanjackson
Copy link
Copy Markdown

I just want to chime in that this is also breaking our build, so we can't use babel-minify 0.2.0 right now. It would be great if we could get a non-canary release that includes this fix.

@vigneshshanmugam
Copy link
Copy Markdown
Member

@allanjackson Its done. use 0.3.0

@allanjackson
Copy link
Copy Markdown

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Tag: Bug Fix Pull Request fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants