[Partial Hydration] Don't invoke listeners on parent of dehydrated event target #16591
Conversation
|
ReactDOM: size: 0.0%, gzip: -0.0% Details of bundled changes.Comparing: 0da7bd0...e50388e react-dom
react-art
react-test-renderer
react-reconciler
react-native-renderer
Generated by 🚫 dangerJS |
|
We need to be able to get to the Fiber from the suspense boundary to be able to schedule a hydration of it. So I also added a "hydrateSuspenseInstance" call to attach an expando that points back to the Suspense fiber instance like we do for other event targets and for DevTools. I moved the logic into getInstanceFromNode so that it now returns either a HostComponent, HostTextComponent or SuspenseComponent. The later is only in the case where the target node is inside a dehydrated suspense tree. We then check if the tag of the returned fiber is a SuspenseComponent. If so, we know to ignore dispatching the event. @bvaughn This will affect DevTools since inspecting a node can now return a SuspenseComponent as the target of that inspection if it's inspected before the tree has hydrated. |
sebmarkbage
force-pushed
the
ignorebeforehydration
branch
from
1f53240
to
b053396
Compare
|
I also added an expando on the container node back to the HostRoot fiber. That way we can also detect if an event happens on anything in the root before we've even hydrated anything (e.g. before calling This also lets us detect a dehydrated Suspense boundary that is directly in the root without any additional DOM nodes. Around it. This also means that EDIT: Looks like this approach doesn't work when the container node is also a React owned DOM node. That's the failing test. |
|
If I'm understanding this right, this will re-target an incoming event to be that of a hydrated/active fiber and its nearest DOM node (now with the additional logic of including the root). If this is the case, is this really the behaviour we want? Shouldn't we proceed with building in event capturing and replaying? In terms of the implementation of what you've built, it looks okay. The failing test likely relates to the quirky way that we handle ancestors with multiple roots. As you've added logic to preacache root fibers on the DOM node, it's over-riding the previously precached non-root fiber that was attached, so it's not able to traverse to the |
Now getClosestInstanceFromNode can return either a host component, host text component or suspense component when the suspense component is dehydrated. We then use that to ignore events on a suspense component.
This lets us detect if an event happens on this root's subtree before it has rendered something.
sebmarkbage
force-pushed
the
ignorebeforehydration
branch
from
e50388e
to
14db1d7
Compare
|
ReactDOM: size: 0.0%, gzip: 0.0% Details of bundled changes.Comparing: f705e2b...1616fe3 react-dom
react-art
react-test-renderer
react-reconciler
react-native-renderer
|
|
I updated the root approach to track a separate expando on the container. That way we can differentiate between a DOM node in its role as a Container and its role as a leaf Instance. Now everything passes. |
We'll need the nearest boundary for event replaying so this prepares for that. This surfaced an issue that we attach Hydrating tag on the root but normally this (and Placement) is attached on the child. This surfaced an issue that this can lead to both Placement and Hydrating effects which is not supported so we need to ensure that we only ever use one or the other.
|
|
||
| export function precacheFiberNode(hostInst, node) { | ||
| node[internalInstanceKey] = hostInst; | ||
| } | ||
|
|
||
| export function markContainerAsRoot(hostRoot, node) { | ||
| node[internalContainerInstanceKey] = hostRoot; |
There was a problem hiding this comment.
I do wonder if its still the best strategy to keep adding additional properties to DOM nodes vs a WeakMap with an object that contains { instance, container, props }. We could probably unify this with the already existing WeakMap we have on DOM nodes for checking for registered events:
This isn't really something for this PR, just an observation.
Update: After looking at this jsperf it might be better if we should stick with a hidden key, but rather than having 3, have a single one and remove the WeakMap and use an object with 4 properties.
There was a problem hiding this comment.
The one for props is an unfortunate one. That was never intentional. I just realized too late I didn't have a convenient place to find the current Fiber. I don't remember exactly why I couldn't just update the fiber pointer to the correct alternate every time events change and then read from memoizedProps. There's probably something we can do to get rid of that one.
Regarding the new container one. It's very rare that something will both be a container and an instance so there will almost never be more than one expando. So for that case it's probably not worth adding an indirection for the very common case just to avoid two slots in the rare case.
|
If I recall correctly we need this expando to fix iOS clicks in portals anyway. |
|
@gaearon Note that this doesn't actually add one for portals. Only roots. Mostly because Portals don't support hydration anyway. |
The container is inside the instance, so we must find it before the instance, since otherwise we'll miss it.
|
I added a note that calling Therefore, to get to a HostRoot, you have to pass a child of the DOM Container node. This isn't really all that important nuance but it does allow me to take some short cuts (e.g. for the common case where the target is an instance) and gives me a convenient hint that a node is indeed dehydrated. |
|
I'll also note for future us, that this has a known problem when the Container is a Comment node. However, that is an unstable feature and currently doesn't work with hydration anyway. So that needs some further consideration (if we ever want to support that use case). |
If an event gets invoked on a child of a suspense boundary we haven't hydrated yet, that's an opportunity where we might want to consider replaying it instead. In the existing semantics there exist no replaying, instead things are ignored (see #16532). So this PR makes sure we don't dispatch these events to React's event system. They may still have been invoked on non-React nodes.
However the tricky part is that we don't readily know if a node is a non-React DOM node or if it's just a node we haven't gotten to yet. We can't just mark the node as dehydrated since the server streaming can update the content of dehydrated boundaries as they go and that would lose the markers.
However, typically there will at least be some React DOM node that is a parent of the Suspense boundary so that will normally become the target today. We also need to deal with the same case when there is a Suspense boundary at the root and while the root most level is concurrently hydrating.
In the case where there is a parent React DOM node, we don't know if the target node was a non-React DOM node that someone manually inserted or if it is a child of a dehydrated boundary.
The common case is that if it's a DOM node that someone messes with, it won't have any children so we can use that as a quick bailout to assume it's not a React node.
If it does have children, I backtrack on the previous siblings to see if we're nested inside a Suspense boundary (i.e. if we're inside two comment nodes). This could potentially be expensive if there are many previous siblings but most of the time there's only one direct DOM node inside a Suspense boundary, and it's unusual that it wouldn't be a Suspense boundary in this case. The worst case is that this is happening on a non-React node and that the React parent happens to have a child that renders null or something in it, and also that this has many children in it.