Skip to content
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

Fix parentheses on replaceWithMultiple for JSX #10598

Merged
merged 3 commits into from Nov 11, 2019

Conversation

khoumani
Copy link
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
Member

CircleCI failure is not related to this PR

@JLHwung JLHwung self-requested a review October 31, 2019 16:49
@@ -97,4 +102,23 @@ describe("path/replacement", function() {
);
});
});
describe("replaceWithMultiple", () => {
it("ReplaceWithMultiple for a JSXElement with a JSXElement parent", () => {
Copy link
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
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
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
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