-
Notifications
You must be signed in to change notification settings - Fork 49.6k
[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
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.
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?
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.
For previousVirtualInstanceWasMount
to be true, we must have already set this below on 3941.
You do set ShouldResetSuspenseChildren
here but you don't on 3941. It's not necessary to set in both places but they should be consistent.
You could maybe check if there was a SuspenseNode added to the previouslyReconciledSiblingSuspenseNode
by checking if it changed before and after mountVirtualInstanceRecursively
to know whether you need to set ShouldResetSuspenseChildren
.
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
1f88d92
to
d10b52b
Compare
d10b52b
to
28cf3ec
Compare
compiledWithForget: boolean, | ||
}; | ||
|
||
export type 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.
Might be an annoying name to work with since we'll have the name imported as <Suspense>
in the same files this will be referenced.
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.
Yeah, I didn't use SuspenseNode to avoid confusing it with the backend type. Maybe that's a smaller issue than confusing it with React.Suspense
?
): void { | ||
const fiberInstance = suspenseInstance.instance; | ||
if (fiberInstance.kind === FILTERED_FIBER_INSTANCE) { | ||
throw new Error('Cannot record a mount for a filtered Fiber instance.'); |
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.
Don't have to do it in this PR but as discussed, we probably should show filtered ones too. It's just that cross-linking to the Components tab would be disabled. That might have implications on how we load the side-panel too.
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.
Yeah, just need to figure out a type pattern because filtered instances have their ID hidden by default.
let unfilteredParent = parentSuspenseInstance; | ||
while ( | ||
unfilteredParent !== null && | ||
unfilteredParent.instance.kind === FILTERED_FIBER_INSTANCE |
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.
We should probably include filtered in a follow up but what we should not include in the front end is boundaries without hasUniqueSuspenders
. They're basically filtered from this tree.
That could potentially be done at the front end level but might be easier and smaller traffic if it's done by the backend just like filtered.
It would just have to reparent or unmount/remount when a previously false boundary becomes true.
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.
Left some minor notes and follow ups.
Stacked on #34082
We could create a separate Suspense store instead. But I found it easier to just reuse the existing operations for now.