Skip to content
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

Merged
merged 2 commits into from
Oct 13, 2020

Conversation

IDrissAitHafid
Copy link
Contributor

@IDrissAitHafid IDrissAitHafid commented Oct 9, 2020

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:
before the fix

After:
after the fix

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 9, 2020

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:

Sandbox Source
React Configuration

@sizebot
Copy link

sizebot commented Oct 9, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 35c7481

@sizebot
Copy link

sizebot commented Oct 9, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 35c7481

@IDrissAitHafid
Copy link
Contributor Author

Here's the code sandbox app of the repro.

@IDrissAitHafid
Copy link
Contributor Author

@bvaughn can you please review this PR 🙏 😅

@bvaughn bvaughn self-assigned this Oct 13, 2020
@bvaughn
Copy link
Contributor

bvaughn commented Oct 13, 2020

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.

@IDrissAitHafid
Copy link
Contributor Author

That's what I thought 😄
Thanks!

@@ -1572,7 +1572,7 @@ export function attach(
if (nextPrimaryChildSet !== null) {
mountFiberRecursively(
nextPrimaryChildSet,
nextFiber,
shouldIncludeInTree ? nextFiber : parentFiber,
Copy link
Contributor

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! 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@bvaughn
Copy link
Contributor

bvaughn commented Oct 13, 2020

Am I understanding correctly that this PR fixes #18256, #19633, #19547, and #18924?

@IDrissAitHafid
Copy link
Contributor Author

I think I didn't explain well what it is fixed 😅
this PR fixed exactly this one #18256, and this one #19633 is always thrown whenever a fiber is missing.

#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.

@bvaughn
Copy link
Contributor

bvaughn commented Oct 13, 2020

Are you confident that #18924 is not fixed? Or are you not confident that it is fixed?

@IDrissAitHafid
Copy link
Contributor Author

IDrissAitHafid commented Oct 13, 2020

I am confident it is not fixed, I still can reproduce it even with this fix.

@bvaughn
Copy link
Contributor

bvaughn commented Oct 13, 2020

Thanks for clarifying! I will reopen for now then :)

@IDrissAitHafid IDrissAitHafid deleted the undefined-fiber branch October 13, 2020 20:32
koto pushed a commit to koto/react that referenced this pull request Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants