-
Notifications
You must be signed in to change notification settings - Fork 49.1k
[DevTools] Send suspense nodes to frontend store #34070
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.
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
base: main
Are you sure you want to change the base?
Conversation
// At least the root should have an associated SuspenseNode. | ||
throw new Error('A SuspenseNode must exist containing this Fiber.'); | ||
} | ||
recordResetSuspenseChildren(suspenseNode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not safe to do this until you've popped back up to the same level as this suspenseNode since otherwise you could call this multiple times within the same parent and too early before all the children have settled into their new place.
One way to do this is to call it unconditionally every time fiberInstance.suspenseNode !== null
here without the parent traversal above.
However, you'd have to call it even if shouldResetChildren
is false. Otherwise you'd miss some cases where the change to the Instance tree is fully handled by some inner virtual or fiber instance. Since shouldResetChildren
only accounts for changes to the DevToolsInstance tree.
It would be semantically correct to always call it. It would just be unnecessary slow since changes deep inside the tree would make the traversal and send the commands to front end every time. The shouldResetChildren
flag only exists as an optimization.
We could return a bitmask that has two flags. shouldResetChildren
and shouldResetSuspenseNodeChildren
up the stack so that shouldResetSuspenseNodeChildren
can bubble up the stack to the nearest SuspenseNode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One way to do this is to call it unconditionally every time fiberInstance.suspenseNode !== null here without the parent traversal above.
I think we need do bubble up a flag for correctness as well. Right now, with the unconditional reset, it doesn't handle this reorder:
<Suspense key="e" name="e">
<Passthrough>
<Suspense name="e-child-one">
<p>e1</p>
</Suspense>
</Passthrough>
<Passthrough>
<Suspense name="e-child-two">
<div>e2</div>
</Suspense>
</Passthrough>
</Suspense>
to
<Suspense key="e" name="e">
<Passthrough>
<Suspense name="e-child-two">
<div>e2</div>
</Suspense>
</Passthrough>
<Passthrough>
<Suspense name="e-child-one">
<p>e1</p>
</Suspense>
</Passthrough>
</Suspense>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented the additional flag. I haven't double checked yet if the "propagate it from everywhere" approach is correct or if I need to be more selected.
The current impl seems to reach parity with the Components tab.
d0c0864
to
d54f79e
Compare
72926ca
to
9a2ace4
Compare
updateFlags |= | ||
ShouldResetChildren | ShouldResetSuspenseChildren; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mountVirtualInstanceRecursively
can mount a Fiber. Was the flag previously not set on purpose or missing?
4efe938
to
c81b79d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a mess at the moment for visual debugging of the store. A follow-up will make this a flat list that you can navigate through.
c81b79d
to
6aff312
Compare
Stacked on #34082
We could create a separate Suspense store instead. But I found it easier to just reuse the existing operations for now.