-
Notifications
You must be signed in to change notification settings - Fork 47.5k
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
handled a missing suspense fiber when suspense is filtered on the profiler #19987
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 35c7481:
|
Here's the code sandbox app of the repro. |
@bvaughn can you please review this PR 🙏 😅 |
Nice fix! I literally just sat down to look into these issues 10 minutes ago, so I appreciate the ping in this case. I didn't notice you'd opened this. |
That's what I thought 😄 |
@@ -1572,7 +1572,7 @@ export function attach( | |||
if (nextPrimaryChildSet !== null) { | |||
mountFiberRecursively( | |||
nextPrimaryChildSet, | |||
nextFiber, | |||
shouldIncludeInTree ? nextFiber : parentFiber, |
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.
Huh. Looks like we handled this correctly in 5 of the 6 places, but missed this one. Great find! 😁
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.
Thank you!
I think I didn't explain well what it is fixed 😅 #18924 is not fixed, when I have time I'll share how to reproduce it and if you want I'll work on it and see if I can find something 😅, this bug #18924 also generates this one #19633. That's why I couldn't tell if #19633 is fixed. |
Are you confident that #18924 is not fixed? Or are you not confident that it is fixed? |
I am confident it is not fixed, I still can reproduce it even with this fix. |
Thanks for clarifying! I will reopen for now then :) |
…filer (facebook#19987) Co-authored-by: Brian Vaughn <[email protected]>
fixes #18256 and fixes #19633
If suspense is filtered in the profiler and started profiling, when the suspense fallback unmounts, the primary fiber tries to mount to the suspense fiber above it, but because it is filtered the profiler throws those errors thus the missing fiber in this #19633 and the undefined in this #18256.
So instead of that, when suspense is filtered I re-parent it to the parent fiber of the suspense fiber.
That said, this problem also generates this issue #19547 and this one #18924. Depending on the place of the suspense fiber in the tree relatively to the rendered elements (is it rendered alone, or rendered with siblings, or only its children are rendered...) a different error is thrown.
In the example below suspense is rendered along with a
<h1>Hello </h1>
sibling and a div parent.Before:
After: