Race condition when swapping nodes between <OutPortal/>s

[antoinetissier]

Problem

Hi, I want to use this library to be able to move some components in a layout, without having to unmount/remount them. However, I am unable to do so because of race conditions when swapping nodes betwing two <OutPortal/>s within a single rerender.

Cause

Here is a very simplified example that shows what happens:

Suppose I have two nodes NodeA and NodeB created once and for all.

Now suppose down the line we use those nodes in a layout, that would look like this at the first render.

<OutPortal node={nodeA}/> // let's call it OutPortal1, and its placeholder Placeholder1
<OutPortal node={nodeB}/> // let's call it OutPortal2, and its placeholder Placeholder2

Then, suppose an update arrives, yielding the following instead:

// nodeA and nodeB have been swapped.
<OutPortal node={nodeB}/>
<OutPortal node={nodeA}/>

Here is roughly what happens when going through the code of react-reverse-portal:
OutPortal1 componentDidUpdate is triggered, with nodeB instead of nodeA

• unmount nodeA
  • OutPortal1.child = Placeholder1
  • nodeA.parent = undefined
• mount nodeB in OutPortal1
  • unmount nodeB
    • OutPortal2.child = Placeholder2
    • nodeB.parent = undefined
  • OutPortal1.child = nodeB
  • nodeB.parent = OutPortal1

OutPortalB componentDidUpdate is triggered with nodeA instead of nodeB

• unmount nodeB (at this point nodeB.parent is OutPortal1)
  • OutPortal1.child = Placeholder2
  • nodeB.parent = undefined
• mount nodeA in OutPortal2
  • unmount nodeA => nothing happens since at this point nodeA.parent is undefined
  • OutPortal2.child = nodeA
  • nodeA.parent = OutPortal2

Because of this race condition, we end up having OutPortal1 yielding Placeholder1 and OutPortal2 yielding NodeA, even though we just wanted to swap the nodes.

Note: this is a very simplified example, but my real use-case involves a recursive layout with potentially many content nodes.

Question

Are there any plans on handling this use case ? Can you think of a workaround ?

[pimterry]

I haven't seen this case before, but it makes sense. The core of the issue is in the unmount nodeB during OutPortalB's componentDidUpdate, right? I'm not affected by this myself, but I'd like to support it, and I'd certainly accept a fix.

It sounds like it should be easy to set up a story to easily reproduce the issue, which would be a good start.

I suspect the underlying bug is that we omit the expectedPlaceholder argument to unmount() in componentDidUpdatehere:

react-reverse-portal/src/index.tsx

Lines 200 to 204 in 088033e

	// If we're switching portal nodes, we need to clean up the current one first.
	if (this.currentPortalNode && node !== this.currentPortalNode) {
	this.currentPortalNode.unmount();
	this.currentPortalNode = node;
	}

Meanwhile we do specify this in componentWillUnmount, precisely because it solves some similar race conditions:

react-reverse-portal/src/index.tsx

Lines 212 to 215 in 088033e

	componentWillUnmount() {
	const node = this.props.node as AnyPortalNode<C>;
	node.unmount(this.placeholderNode.current!);
	}

I haven't checked, but you can probably just change the former to also pass this.placeholderNode.current!. If you do so then unmount will check that the most recent call to mount was passed that same placeholder (i.e. came from the same OutPortal), and will do nothing if not (i.e. if node has already been mounted elsewhere in the meantime).

That expectedPlaceholder param is optional because it needs to be omitted when unmount is called from within mount (which wants to always unmount, no matter where the node is right now), but I don't think there's a good reason why it's not specified here, it's just a missed case.

Can you give that a go?

[antoinetissier]

Thanks for your answer, I will try it and keep you updated

[antoinetissier]

It seems to work well ! Here is the PR #16

[pimterry]

Fix is now merged and released and it sounds like it solves your issue so I'm going to close this, but do shout if you're still seeing issues related to this.

[antoinetissier]

I tested it on my own project and it works well 👍 Thanks for your quick answer and release