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

Update pathCache in NodePath#_replaceWith #12391

Merged
merged 5 commits into from Nov 24, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -0,0 +1,2 @@
const a = 1;
const b = () => a?.b?.c!.d;
@@ -0,0 +1,3 @@
{
"plugins": ["transform-typescript", "proposal-optional-chaining"]
}
@@ -0,0 +1,7 @@
const a = 1;

const b = () => {
var _a$b;

return a === null || a === void 0 ? void 0 : (_a$b = a.b) === null || _a$b === void 0 ? void 0 : _a$b.c.d;
};
3 changes: 2 additions & 1 deletion packages/babel-traverse/src/path/replacement.js
Expand Up @@ -50,7 +50,7 @@ export function replaceWithMultiple(nodes: Array<Object>) {
nodes = this._verifyNodeList(nodes);
t.inheritLeadingComments(nodes[0], this.node);
t.inheritTrailingComments(nodes[nodes.length - 1], this.node);
pathCache.get(this.parent).delete(this.node);
pathCache.get(this.parent)?.delete(this.node);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fix is inspired from ancestry commit that in some user cases the pathCache is nullish in NodePath#_replaceWith.

this.node = this.container[this.key] = null;
const paths = this.insertAfter(nodes);

Expand Down Expand Up @@ -199,6 +199,7 @@ export function _replaceWith(node) {
}

this.debug(`Replace with ${node?.type}`);
pathCache.get(this.parent)?.set(node, this).delete(this.node);

this.node = this.container[this.key] = node;
}
Expand Down
8 changes: 8 additions & 0 deletions packages/babel-traverse/test/conversion.js
Expand Up @@ -51,6 +51,14 @@ describe("conversion", function () {
expect(generateCode(rootPath)).toBe("() => {\n return false;\n};");
});

it("preserves arrow function body's context", function () {
Copy link
Member

Choose a reason for hiding this comment

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

Do you need another test, or is this enough? (Since there is the to-do item in the PR description)

I can work on it if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is copied from the replaceWith test above, see also #12391 (comment). The second commit fixes another regression introduced in #12302 that is not yet reported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can work on it if needed.

Appreciated. Feel free to push 😄. I am still trying to figure out how we should test the pathCache behaviour in @babel/traverse.

const rootPath = getPath("() => true").get("expression");
const body = rootPath.get("body");
rootPath.ensureBlock();
body.replaceWithMultiple([t.booleanLiteral(false), t.emptyStatement()]);
expect(generateCode(rootPath)).toBe("() => {\n return false;\n};");
});

it("converts for loop with statement body to block", function () {
const rootPath = getPath("for (;;) true;");
rootPath.ensureBlock();
Expand Down
27 changes: 27 additions & 0 deletions packages/babel-traverse/test/replacement.js
Expand Up @@ -112,6 +112,33 @@ describe("path/replacement", function () {
});
expect(visitCounter).toBe(1);
});

// https://github.com/babel/babel/issues/12386
it("updates pathCache with the replaced node", () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I come up with a slightly simplified test from the regression example: 8d8782a#diff-f6b6fd6b98ff2127cf882c020e42e695555b277d7073252187299fb7c3f875a2

This test does not depend on the behaviour of proposal-optional-chaining and transform-typescript.

const ast = parse(`() => (a?.b)?.c`, {
createParenthesizedExpressions: true,
});
traverse(ast, {
OptionalMemberExpression(path) {
path.node.type = "MemberExpression";
// force `replaceWith` to replace `path.node`
path.replaceWith(path.node.__clone());
path.parentPath.ensureBlock();

const aQuestionDotBNode = path.node.object.expression;
// avoid traversing to a?.b
aQuestionDotBNode.type = "MemberExpression";
},
ParenthesizedExpression(path) {
path.replaceWith(path.node.expression);
},
});
expect(generate(ast).code).toMatchInlineSnapshot(`
"() => {
return a.b.c;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As expected v7.12.7 returns

() => {
          return (a.b).c;
};

here, which means the test captures the behaviour difference from the path cache.

};"
`);
});
});
describe("replaceWithMultiple", () => {
it("does not add extra parentheses for a JSXElement with a JSXElement parent", () => {
Expand Down