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

Bug: SuspenseList revealOrder="together" and error boundaries #18429

Closed
eps1lon opened this issue Mar 29, 2020 · 4 comments
Closed

Bug: SuspenseList revealOrder="together" and error boundaries #18429

eps1lon opened this issue Mar 29, 2020 · 4 comments

Comments

@eps1lon
Copy link
Collaborator

eps1lon commented Mar 29, 2020

React version: 0.0.0-experimental-aae83a4b9

Steps To Reproduce

  1. Wrap and error boundary around a suspense boundary inside a <SuspenseList revealOrder="together" />
  2. Crash with Cannot read property 'shared' of null

Link to code example:

https://codesandbox.io/s/quirky-cannon-e4t1c is forked from https://codesandbox.io/s/black-wind-byilt

The current behavior

The following code causes the described crash:

<SuspenseList revealOrder="together">
  <ErrorBoundary fallback={null}>
    <Suspense fallback={<h2>Loading posts...</h2>}>
      <ProfileTimeline resource={resource} />
    </Suspense>
  </ErrorBoundary>
  <Suspense fallback={<h2>Loading fun facts...</h2>}>
    <ProfileTrivia resource={resource} />
  </Suspense>
</SuspenseList>

revealOrder="forwards" does not crash as well as placing the error boundary inside the suspense boundary like so:

<SuspenseList revealOrder="together">
  <Suspense fallback={<h2>Loading posts...</h2>}>
    <ErrorBoundary fallback={null}>
      <ProfileTimeline resource={resource} />
    </ErrorBoundary>
  </Suspense>
  <Suspense fallback={<h2>Loading fun facts...</h2>}>
    <ProfileTrivia resource={resource} />
  </Suspense>
</SuspenseList>

The expected behavior

The position of the ErrorBoundary is likely wrong in the first place (since I can move it around to not cause a crash while preserving the semantics I expect). Maybe this needs a descriptive warning/error why this is happening and how I should resolve it?

@eps1lon eps1lon added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Mar 29, 2020
@gaearon gaearon added Component: Suspense Type: Bug and removed Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug labels Mar 29, 2020
@gaearon
Copy link
Collaborator

gaearon commented Mar 29, 2020

Sounds like a bug.

@gaearon
Copy link
Collaborator

gaearon commented Mar 29, 2020

Would you mind sending this as a PR with a failing test? You can see existing SuspenseList tests as an inspiration.

@eps1lon
Copy link
Collaborator Author

eps1lon commented Mar 29, 2020

Added test in #18432

This is reproducible with a basic class component which means the assumption in

// This is always non-null on a ClassComponent or HostRoot
const queue: UpdateQueue<State> = (workInProgress.updateQueue: any);

does not hold. This guard was removed in 7bf40e1#diff-b69c8f2165f95ef2355ac661c12fa407L867

@eps1lon
Copy link
Collaborator Author

eps1lon commented Apr 1, 2020

Closed in #18448

@eps1lon eps1lon closed this as completed Apr 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants