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.

Sign up for GitHub

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

Jump to bottom

Update pathCache in NodePath#_replaceWith #12391

Merged
merged 5 commits into from Nov 24, 2020
Merged

Update pathCache in NodePath#_replaceWith #12391

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8d8782a
add tests on regression 12386
existentialism Nov 23, 2020
3949b40
fix: update cache on _replaceWith
JLHwung Nov 24, 2020
04d106c
fix: pathCache in replaceWithMultiple could be nullish
JLHwung Nov 24, 2020
9b91f0f
Update packages/babel-traverse/src/path/replacement.js
JLHwung Nov 24, 2020
307b003
test: add replaceWith test to traverse
JLHwung Nov 24, 2020
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
2 changes: 1 addition & 1 deletion packages/babel-traverse/src/path/replacement.js
View file
Open in desktop
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
8 changes: 8 additions & 0 deletions packages/babel-traverse/test/conversion.js
View file
Open in desktop
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