Skip to content

test: add tests about behaviour of replaceWithMultiple #12309

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
Merged
JLHwung merged 2 commits into from
Nov 4, 2020
Merged

test: add tests about behaviour of replaceWithMultiple #12309

JLHwung merged 2 commits into from
Nov 4, 2020

Conversation

@JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Nov 4, 2020 •
edited by gitpod-io bot
Loading

Q                       A
Tests Added + Pass? Yes
License MIT

Add a test on the NodePath#replaceWithMultiple behaviour when one of node is the replaced node. This tests is required by #12302, I feel it should be landed separately.

This behaviour is not intuitive to me. It is not visited because in

babel/packages/babel-traverse/src/path/replacement.js

Lines 46 to 52 in 12c6db6

export function replaceWithMultiple(nodes: Array<Object>) {
this.resync();
nodes = this._verifyNodeList(nodes);
t.inheritLeadingComments(nodes[0], this.node);
t.inheritTrailingComments(nodes[nodes.length - 1], this.node);
this.node = this.container[this.key] = null;
,

when we clear this.node (we denote this.node by n, w.l.o.g. we assume nodes[0] === n), we don't update the pathCache of its parent.

babel/packages/babel-traverse/src/path/index.js

Line 70 in 12c6db6

const paths = pathCache.get(parent) || [];

Since the pathCache holds a NodePath which has a reference to the n, it implicitly copy n to a new memory address. Now when the n is queried against pathCache

babel/packages/babel-traverse/src/path/index.js

Line 79 in 12c6db6

if (pathCheck.node === targetNode) {

The pathCache is missed because the NodePath is referencing a shallow copy of n, so a new NodePath instance is created for n and this.node is always null during

babel/packages/babel-traverse/src/path/replacement.js

Lines 53 to 59 in 12c6db6

const paths = this.insertAfter(nodes);
if (this.node) {
this.requeue();
} else {
this.remove();
}

then it is removed instead of requeue so this node is never visited after replaced.

I think intuitively all nodes in the argument of replaceWithMultiple should be revisited. But the suggested behaviour is a breaking change, e.g. breaking TDZ implementation.

@JLHwung JLHwung added area: tests PR: Internal 🏠 A type of pull request used for our changelog categories pkg: traverse labels Nov 4, 2020
@babel-bot
Copy link
Collaborator

babel-bot commented Nov 4, 2020 •
edited
Loading

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

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 4, 2020 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 0eb5214:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

existentialism
existentialism approved these changes Nov 4, 2020
View reviewed changes
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.

💯

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Nov 4, 2020

What happens if path.node is not the first element in replaceWithMultiple?

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Nov 4, 2020

I think intuitively all nodes in the argument of replaceWithMultiple should be revisited. But the suggested behaviour is a breaking change, e.g. breaking TDZ implementation.

I'm not sure that I would expect that behavior. However, I think it should match whatever path.replaceWith(path.node) does. My intuition is that neither should requeue the node, unless someone does path.requeue() explicitly.

@JLHwung
Copy link
Contributor Author

JLHwung commented Nov 4, 2020

What happens if path.node is not the first element in replaceWithMultiple?

The order does not matters since they are created in a loop

babel/packages/babel-traverse/src/path/modification.js

Lines 57 to 65 in 12c6db6

for (let i = 0; i < nodes.length; i++) {
const to = from + i;
const path = this.getSibling(to);
paths.push(path);
if (this.context && this.context.queue) {
path.pushContext(this.context);
}
}

but I have added another tests.

However, I think it should match whatever path.replaceWith(path.node) does. My intuition is that neither should requeue the node, unless someone does path.requeue() explicitly.

Yeah NodePath#replaceWith does not requeue when this.node equals to the replacement,

babel/packages/babel-traverse/src/path/replacement.js

Lines 119 to 121 in 12c6db6

if (this.node === replacement) {
return [this];
}

but it does requeue for other cases.

babel/packages/babel-traverse/src/path/replacement.js

Lines 178 to 179 in 12c6db6

// requeue for visiting
this.requeue();

@JLHwung JLHwung force-pushed the add-test-on-replace-with-multiple branch from 942a37e to 0eb5214 Compare November 4, 2020 19:35
@JLHwung JLHwung merged commit 6cb6f9f into babel:main Nov 4, 2020
@JLHwung JLHwung deleted the add-test-on-replace-with-multiple branch November 4, 2020 19:46
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Feb 4, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@existentialism existentialism existentialism approved these changes

@nicolo-ribaudo nicolo-ribaudo nicolo-ribaudo approved these changes

Assignees

No one assigned

Labels

area: tests outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: traverse PR: Internal 🏠 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.

4 participants

@JLHwung @babel-bot @nicolo-ribaudo @existentialism