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

[suspense][error handling] Add failing unit test #16800

Closed
wants to merge 1 commit into from

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Sep 17, 2019

Covers an edge case where an error is thrown inside the complete phase of a component that is in the return path of a component that suspends. The second error should also be handled (i.e. able to be captured by an error boundary.

The test is currently failing because there's a call to completeUnitOfWork inside the main render phase catch block. That call is not itself wrapped in try-catch, so anything that throws is treated as a fatal/unhandled error.

I believe this bug is only observable if something in the host config throws; and, only in legacy mode, because in concurrent/batched mode, completeUnitOfWork on fiber that throws follows the "unwind" path only, not the "complete" path, and the "unwind" path does not call any host config methods.

I have a fix, but it overlaps with a refactoring PR that I'm also working on, so I'm going to stack it on top of that.

Covers an edge case where an error is thrown inside the complete phase
of a component that is in the return path of a component that suspends.
The second error should also be handled (i.e. able to be captured by
an error boundary.

The test is currently failing because there's a call to
`completeUnitOfWork` inside the main render phase `catch` block. That
call is not itself wrapped in try-catch, so anything that throws is
treated as a fatal/unhandled error.

I believe this bug is only observable if something in the host config
throws; and, only in legacy mode, because in concurrent/batched mode,
`completeUnitOfWork` on fiber that throws follows the "unwind" path
only, not the "complete" path, and the "unwind" path does not call
any host config methods.
@sizebot
Copy link

sizebot commented Sep 17, 2019

No significant bundle size changes to report.

Generated by 🚫 dangerJS against 5b9991d

// Covers an edge case where an error is thrown inside the complete phase
// of a component that is in the return path of a component that suspends.
// The second error should also be handled (i.e. able to be captured by
// an error boundary.

Choose a reason for hiding this comment

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

Suggested change
// an error boundary.
// an error boundary.)

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

Test seems reasonable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants