Skip to content

Fix parentheses on replaceWithMultiple for JSX#10598

Merged
nicolo-ribaudo merged 3 commits intobabel:masterfrom
khoumani:issues9851
Nov 11, 2019
Merged

Fix parentheses on replaceWithMultiple for JSX#10598
nicolo-ribaudo merged 3 commits intobabel:masterfrom
khoumani:issues9851

Conversation

@khoumani
Copy link
Copy Markdown
Contributor

Q                       A
Fixed Issues? Fixes #9851
Tests Added + Pass? Yes
Any Dependency Changes? No
License MIT

replaceWithMultiple() first sets the node to replace to null then add the new nodes after it through insertAfter().

insertAfter() already checks for when the node is a JSXElement so that it proceeds to inserting the new nodes to the container. To fix the issue, we want to also check if the parent is a JSXElement.

@khoumani khoumani marked this pull request as ready for review October 23, 2019 22:12
@nicolo-ribaudo nicolo-ribaudo added area: jsx pkg: traverse PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Oct 23, 2019
@JLHwung JLHwung changed the title Closes #9851 Fix parentheses on replaceWithMultiple for JSX Fix parentheses on replaceWithMultiple for JSX Oct 24, 2019
@nicolo-ribaudo
Copy link
Copy Markdown
Member

CircleCI failure is not related to this PR

@JLHwung JLHwung self-requested a review October 31, 2019 16:49
});
});
describe("replaceWithMultiple", () => {
it("ReplaceWithMultiple for a JSXElement with a JSXElement parent", () => {
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.

Suggested change
it("ReplaceWithMultiple for a JSXElement with a JSXElement parent", () => {
it("does not add extra parenthesis for a JSXElement with a JSXElement parent", () => {

Copy link
Copy Markdown
Member

@existentialism existentialism left a comment

Choose a reason for hiding this comment

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

One small nit, otherwise LGTM!

@JLHwung
Copy link
Copy Markdown
Contributor

JLHwung commented Nov 9, 2019

CI error is unrelated.

@nicolo-ribaudo nicolo-ribaudo merged commit 2b08260 into babel:master Nov 11, 2019
@nicolo-ribaudo
Copy link
Copy Markdown
Member

Thanks!

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

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Babel's “replaceWithMultiple” adds unnecessary parentheses

4 participants