Bug: CM double rendering affects how the renderer deals with events

[drcmda]

My custom renderer registers events in the createInstance phase: https://github.com/pmndrs/react-three-fiber/blob/master/src/renderer.tsx#L359 it recognizes events of a certain type and stores them on the main instance. i believe this is also similar to how art does it: https://github.com/facebook/react/blob/master/packages/react-art/src/ReactARTHostConfig.js#L284 The instance will later go through its event-handler array to call them when needed.

The problem

Concurrent mode double renders the view, creates all the instances twice, adds the events twice, but only appends the second fragment to the container node. The first fragment is left hanging. It has written its events into the main instance, but it is not taken away nor is removeChild called on it. The main instance has both events listed as active and will execute them both, it does not know that one leads to a dead fragment.

Demo

Demo to reproduce: https://codesandbox.io/s/react-three-fiber-gestures-forked-v6vyv (click the box, it has two "up" events in the console)

From looking through the react-art source-code, it should exhibit the same problem.

Solution

I can understand that components need to be pure. But the renderer needs to mutate and store stuff. The reconciler should give some indication that a created instance is invalid, so that its internal effects can be removed.

If this turns out to be not a bug, how should the renderer store the events? If we take react-art as a reference it adds them in createInstance and removes them on removeChild.

[vkurchatkin]

Concurrent mode double renders the view,

Your example doesn't use Concurrent Mode though

[drcmda]

the custom renderer is put into cm via the <Canvas concurrent> flag.

Renderer.createContainer(
  container,
  state.current.concurrent ? 2 : 0,
  false,
  null
))

react-dom just bootstraps the canvas, it plays no other role.

[gaearon]

But the renderer needs to mutate and store stuff. The reconciler should give some indication that a created instance is invalid, so that its internal effects can be removed.

The reconciler assumes that the renderer is modeled after the DOM API. In DOM, createInstance is implemented as document.createElement. It is safe to call addEventListener() on the created element even if it doesn't end up in the DOM, simply because if it's not in the DOM, it will not receive events.

So if the events are being registered on the instance itself (rather than on the container), I don't see why creating nodes and throwing them away would be a problem. This is definitely not a problem in the DOM:

const div = document.createElement('div')
div.addEventListener('click', function() {
  alert('hi')
})
// This div has no effect on your app and will be GC'd.

I don't know how React ART or React Three works for sure, so maybe registering events does something to the container instead. For example, this is actually how delegation works in React DOM. (Although it technically doesn't have to be — that's an extra layer of optimization that React DOM does.)

To make this work, React DOM registers all events at the container level. It eagerly runs addEventListener for every known event at the root container node. During createInstance and commitUpdate it updates each node's field that tracks the current props, like here. Finally, when the root container catches an event, React walks the Fiber tree from the node that caught the event upward, and collects all listeners from each node's current props. This is how it knows which listeners to fire.

---

Now, the question of what you should do depends on what APIs Three.js exposes. It's not obvious for me how it's hooked up from React Three. I see you're adding something to __handlers but it's not clear to me how that gets hooked up to the actual DOM listener. Maybe you could tell a bit more about how that works?

I imagine that the simplest solution is to keep handlers on the node, like you already do, but we need to figure out why those handlers somehow "leak to the root" even if that createInstance result never gets attached to the tree. But to answer this question I would need a walkthrough of how that is happening today.

[gaearon]

I've taken a quick look and it appears that __handlers is unrelated to Three.js and you're managing it in this file:

https://github.com/pmndrs/react-three-fiber/blob/1add5008572fab49b669f0ff0644801f41cad752/src/canvas.tsx#L354

(and a few other places in it)

The question is then why does your hit testing include nodes that have been created but never added to the tree. As long as your createInstance doesn't add anything to the tree, these nodes shouldn't be visible to your hit testing, and thus they shouldn't fire events. So that's what you need to figure out.

[gaearon]

By the way, out of curiosity, here's a way to reproduce the same problem without Concurrent Mode:

https://codesandbox.io/s/react-three-fiber-gestures-forked-7hphl?file=/src/App.js

Here, we render two components side by side, and the second one throws. An error boundary catches the error and displays a fallback. But because of the same issue, the "dead" box (which should never been attached to the root tree) somehow still receives the events. What's attaching it to the tree?

[drcmda]

Thanks for looking into it!

The question is then why does your hit testing include nodes that have been created but never added to the tree.

How can i know that this happens, say for a nested object. It does get appendChild, but it doesn't know that the root element has been added into the real visual tree. So the double-render is technically also appended.

<group> // one version is appended into the visual tree, another isn't
  <mesh onClick={...} /> // this node is always appended, double render and real version

One idea i have is that i could maybe use something like appendChildToContainer and then traverse children and write their events into the main instance. In the above example the group for instance would be responsible to traverse children and add them. Is this what you mean, and is this the right reconciler-function?

What you should do depends on what APIs Three.js exposes

That's the thing. Three does not have events, not even a concept of it. I modelled this after the art-renderer which im guessing has the same problem. The reason i store events on the main instance is because the central raycaster (a device that shoots a ray and picks up hit objects) must only check objects that have handlers on them, it can't pierce the entire scene because that would be very expensive.

[gaearon]

appendChild alone cannot be the problem either. React should never append a child to a parent that’s already in the tree unless that child node is definitely gonna be in the tree as well.

There are two situations where React appends nodes to the tree:

• When some subtree is being rendered for the first time, React will createInstance those nodes and appendInitialChild them to each other. However this does not connect them to the existing tree yet. Yes, some of those nodes might get thrown away if you suspend, but that’s expected. By this point that new subtree is fully disconnected.

• When the whole subtree is ready, it will attach the new tree to the existing one. At that point React does appendChild, insertBefore, or their *In/ToContainer* versions. Here, React provides a guarantee that it will not attach a “dead” instance.

This approach is intended to give you a guarantee that the first step only creates a disconnected tree (so no events can fire on it), and the second step connects it (by which point the tree is guaranteed to not be thrown away). The question is what’s happening differently that’s apparently causing your tree to be connected to the container in the first stage.

[gaearon]

In other words, in the render phase the renderer only creates nodes, sets their properties and attaches them to each other. But because that phase should have no side effects observable to the user, it does not do anything to the tree that’s already on the screen.

But in the commit phase, React actually attaches the newly created tree to the existing one (which is fast because the whole subtree has already been prepared).

I hope this clarifies the note about purity. Of course the renderer itself does mutations, but there is also a certain order to them.

[gaearon]

OK this looks like the likely culprit:

https://github.com/pmndrs/react-three-fiber/blob/1add5008572fab49b669f0ff0644801f41cad752/src/renderer.tsx#L284

We have a function that's also called while creating the initial subtree — applyProps. But that function changes the fields on container. As a result, even if that three is discarded (due to an error or suspending), it has already "left its mark" on the container. That is not OK.

I see a few solutions here.

Solution 1: attach all "interactions" during the commit

If you return true from finalizeInitialChildren for the nodes that require this (i.e. nodes with interactions), you will receive a commitMount(instance, type, props) call for each of them when the subtree they're in gets connected to the real tree. This is where you can modify the container and update its __interactions field.

Solution 2: search for interactive nodes during the event

Instead of maintaining a flat array and using it when receiving the event, you could do a recursive tree traversal during the event to collect the nodes with interactions.

Picking a solution

I guess that in your case the majority of nodes are non-interactive, so it would be unnecessary to always search over them. So the first solution makes sense to me, if it works. Give it a try?

[drcmda]

Nice! I really like solution 1, it seems like commitMount is exactly what i need - i didn't know it existed. Solution 2 would be expensive since trees could get very large. Will try it now ...

[gaearon]

Solution 1 isn’t completely free either since it’s adding some work in the commit phase. It should be ok though.

If neither solution seems very promising we can brainstorm more. But let’s try the commitMount route first.

[gaearon]

By the way what you’re describing seems unrelated to double-rendering in CM. Like I explained in another thread, double-rendering does not create tree instances. It only calls your function twice (and discards one of the results).

What you’re seeing in this issue is related to using Suspense. In particular, to throwing away the tree because rendering was interrupted.

I don’t remember off the top of my head why you don’t see the same behavior in legacy mode. But I’m assuming it might be because of some quirk of legacy Suspense that masks this problem. (But causes others, like #14536.)

[drcmda]

It works, thank you! And it's a clean solution afaic! I can see how avoiding side-effects in the renderer itself makes sense. I know you are all busy, i'm just curious - do you think react-reconciler will be documented one day? I guess other projects may run into this once CM is enabled in them, for instance react-konva seems to use a similar pattern https://github.com/konvajs/react-konva/blob/master/src/makeUpdates.js#L121-L127 createInstance > applyProps > set events on the central root instance

[gaearon]

I don't think full documentation will be tenable because the config still changes, and will likely expand rapidly (and continue changing) when we get to layout animations. But maybe it makes sense to document at least the core methods since those have barely changed for the past couple of years. I guess I can do that now.

[drcmda]

Btw @gaearon what stops me from always returning true in finalizeInitialChildren and applying first props in commitMount? is there any disadvantage?

[gaearon]

Yes, you'd be shifting more work to commit phase, negating the benefits of Concurrent Mode. Anything that doesn't produce a side effect on the existing tree should be done in the render phase so that it can be time-sliced. This keeps the commits themselves short. If more work is shifted into the commit phase, commits become longer, defeating the purpose.

[gaearon]

OK there you go. #20278

[gaearon]

https://github.com/facebook/react/tree/master/packages/react-reconciler#an-incomplete-reference