test: add tests about behaviour of replaceWithMultiple

[JLHwung]

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.

[babel-bot]

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

[codesandbox-ci]

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]

💯

[nicolo-ribaudo]

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

[nicolo-ribaudo]

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]

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();