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
Wrap retrySuspendedRoot using SchedulerTracing #13776
Conversation
@@ -2631,6 +2646,11 @@ describe('Profiler', () => { | |||
}, | |||
); | |||
}); | |||
expect(ReactTestRenderer).toHaveYielded([ |
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.
The timing of these yields is unchanged; the old test was misleading.
expect(renderer).toFlushAndYield(['AsyncText [loaded]']); | ||
expect(renderer.toJSON()).toEqual(['loaded', 'updated']); | ||
|
||
expect(onRender).toHaveBeenCalledTimes(1); | ||
expect(onRender.mock.calls[0][6]).toMatchInteractions([ | ||
initialRenderInteraction, | ||
highPriUpdateInteraction, |
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.
The way this test changed is a little interesting. Our "hi-pri update" causes us to re-call AsyncText which means that in a sense, the update now is blocked on the suspended component before it fully resolves. I can see the argument either way for how to account for this. The good news is that if we only scheduled an update on the sibling then they would stay independent.
ReactDOM: size: 0.0%, gzip: 0.0% Details of bundled changes.Comparing: 40a521a...f9f43af react-dom
react-art
react-test-renderer
react-reconciler
react-native-renderer
scheduler
Generated by 🚫 dangerJS |
d77a2bd
to
21a215a
Compare
@@ -766,7 +761,7 @@ function commitRoot(root: FiberRoot, finishedWork: Fiber): void { | |||
unhandledError = error; | |||
} | |||
} finally { | |||
if (!nextRenderIncludesTimedOutPlaceholder) { | |||
if (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.
oops
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.
ok whatever, ANdrew
e1b4d02
to
3501994
Compare
Previously, we were emptying root.pendingInteractionMap and permanently losing those interactions when applying an unrelated update to a tree that has no scheduled work that is waiting on promise resolution. (That is, one that is showing a fallback and waiting for the suspended content to resolve.) The logic I'm leaving untouched with `nextRenderIncludesTimedOutPlaceholder` is *not* correct -- what we want is instead to know if *any* placeholder anywhere in the tree is showing its fallback -- but we don't currently have a better replacement, and this should unblock tracing with suspense again.
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.
I think this makes sense. A little nervous since this heuristic has changed several times now, but tests give me confidence. Let's sync it and see what happens :D
Will follow-up on the bug you found about multiple pings to a timed-out placeholder.
3501994
to
f9f43af
Compare
Previously, we were emptying root.pendingInteractionMap and permanently losing those interactions when applying an unrelated update to a tree that has no scheduled work that is waiting on promise resolution. (That is, one that is showing a fallback and waiting for the suspended content to resolve.) The logic I'm leaving untouched with `nextRenderIncludesTimedOutPlaceholder` is *not* correct -- what we want is instead to know if *any* placeholder anywhere in the tree is showing its fallback -- but we don't currently have a better replacement, and this should unblock tracing with suspense again.
Previously, we were emptying root.pendingInteractionMap and permanently losing those interactions when applying an unrelated update to a tree that has no scheduled work that is waiting on promise resolution. (That is, one that is showing a fallback and waiting for the suspended content to resolve.) The logic I'm leaving untouched with `nextRenderIncludesTimedOutPlaceholder` is *not* correct -- what we want is instead to know if *any* placeholder anywhere in the tree is showing its fallback -- but we don't currently have a better replacement, and this should unblock tracing with suspense again.
Previously, we were emptying root.pendingInteractionMap and permanently losing those interactions when applying an unrelated update to a tree that has no scheduled work that is waiting on promise resolution. (That is, one that is showing a fallback and waiting for the suspended content to resolve.) The logic I'm leaving untouched with `nextRenderIncludesTimedOutPlaceholder` is *not* correct -- what we want is instead to know if *any* placeholder anywhere in the tree is showing its fallback -- but we don't currently have a better replacement, and this should unblock tracing with suspense again.
Previously, we were emptying root.pendingInteractionMap and permanently losing those interactions when applying an unrelated update to a tree that has no scheduled work that is waiting on promise resolution. (That is, one that is showing a fallback and waiting for the suspended content to resolve.)
The logic I'm leaving untouched with
nextRenderIncludesTimedOutPlaceholder
is not correct -- what we want is instead to know if any placeholder anywhere in the tree is showing its fallback -- but we don't currently have a better replacement, and this should unblock tracing with suspense again.