Skip to content

Fix a regression introduced in #7040#7116

Merged
nicolo-ribaudo merged 1 commit intobabel:masterfrom
nicolo-ribaudo:issue-7114
Dec 28, 2017
Merged

Fix a regression introduced in #7040#7116
nicolo-ribaudo merged 1 commit intobabel:masterfrom
nicolo-ribaudo:issue-7114

Conversation

@nicolo-ribaudo
Copy link
Copy Markdown
Member

@nicolo-ribaudo nicolo-ribaudo commented Dec 27, 2017

Q                       A
Fixed Issues? #7114
Patch: Bug Fix? 👍
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR
Any Dependency Changes?
License MIT

This PR fixes a regression introduced in #7040. I actually wrote some tests to prevent this, but they where wrong 🤣

Example:

// Input
export default x;

// Plugin
pathToThatX.insertAfter(t.identifier("y"));

// Old output (wrong)
export default x;
y;

// Correct output
export default (_tmp = x, y, _tmp);

They might look the same, but when using path.replaceWithMultiple (which removes the node and then inserts the new nodes after), it generated something like

export default ;
y;

And export default got removed since it is invalid.

@nicolo-ribaudo nicolo-ribaudo added 7.x: regression PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Dec 27, 2017
yavorsky
yavorsky approved these changes Dec 27, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@yavorsky yavorsky left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the quick fix 👍

@babel-bot
Copy link
Copy Markdown
Collaborator

babel-bot commented Dec 27, 2017 •
edited
Loading

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/6345/

@Jessidhia Jessidhia mentioned this pull request Dec 28, 2017
Closed
@nicolo-ribaudo nicolo-ribaudo merged commit 2297e2d into babel:master Dec 28, 2017
@nicolo-ribaudo nicolo-ribaudo deleted the issue-7114 branch December 28, 2017 21:16
@Andarist
Copy link
Copy Markdown
Member

Andarist commented Jan 2, 2018 •
edited
Loading 

// Old output (wrong)
export default x;
y;

seems actually more intuitive for me, than inserting they y kinda in between like in

// Correct output
export default (_tmp = x, y, _tmp);

I understand that path.replaceWithMultiple and its usage of path.insertAfter under the hood caused issues, but maybe the fix should be done in the replaceWithMultiple and not in the insertAfter/insertBefore.

@nicolo-ribaudo
Copy link
Copy Markdown
Member Author

nicolo-ribaudo commented Jan 2, 2018

Well, that is how we already do with expressions: https://astexplorer.net/#/gist/226f5986f037c3e32844ef860aa0bbc6/a927881f46355e9d1f1817ad666e65ba34825f51

@nicolo-ribaudo nicolo-ribaudo mentioned this pull request Jan 3, 2018
Closed
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@existentialism existentialism existentialism approved these changes

@yavorsky yavorsky yavorsky approved these changes

Assignees

No one assigned

Labels

7.x: regression outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 A type of pull request used for our changelog categories

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@nicolo-ribaudo @babel-bot @Andarist @existentialism @yavorsky