Bug: Nested SuspenseList may display fallbacks while the component is loaded on re-render

[dubzzz]

React version: 0.0.0-experimental-e5d06e34b

Hoping this can be helpful for you. Here is what looks to be a bug with concurrent mode and nested SuspenseList.

Steps To Reproduce

In concurrent mode only

0. Setup

Let's suppose we have three components <A /> (not lazy loaded), <B /> (not lazy loaded) and <C /> (lazy loaded).

In other words:

import A from './A';
import B from './B';
const C = React.lazy(() => import('./C'));

1. Render

render(
  <SuspenseList key="1" revealOrder="forwards">
    <SuspenseList key="1.1" revealOrder="forwards">
      <Suspense key="1.1.a" fallback={<div>Loading A</div>}>
        <A />
      </Suspense>
    </SuspenseList>
  </SuspenseList>
)

2. Update the component (component now shows A, B, and C)

render(
  <SuspenseList key="1" revealOrder="forwards">
    <SuspenseList key="1.1" revealOrder="forwards">
      <Suspense key="1.1.a" fallback={<div>Loading A</div>}>
        <A />
      </Suspense>
      <Suspense key="1.1.b" fallback={<div>Loading B</div>}>
        <B />
      </Suspense>
      <Suspense key="1.1.c" fallback={<div>Loading C</div>}>
        <C />
      </Suspense>
    </SuspenseList>
  </SuspenseList>
)

3. Output is: A / Loading B / Loading C. While B has already been loaded (not lazy loaded). If I understand well the behaviour of forwards I would have expect to have A / B / Loading C instead.

Please note that the behaviour is not the same if I do not use nested <SuspenseList />.

CodeSandbox: https://codesandbox.io/s/mutable-rain-3ikor
GitHub pages repro: https://dubzzz.github.io/react-suspenselist-bug/build/
GitHub pages source code: https://github.com/dubzzz/react-suspenselist-bug

The current behavior

Output is: A / Loading B / Loading C with nested SuspenseList

The expected behavior

Output is: A / B / Loading C with nested SuspenseList

How did I found this bug?

This potential bug has been discovered while I was trying to run property based tests based on fast-check against React library.

See dubzzz@e2cb477

[gaearon]

Haven’t looked at the issue yet (and might not for a bit). But we need a lot stronger fuzz coverage for Suspense, SuspenseList, and priority scheduling. See the recent closed bugs with “concurrent mode” label for examples of how things go wrong and what catches them. If you’d like to contribute, coming up with a stronger fuzz and submitting failing tests would be much appreciated.

[gaearon]

For SuspenseList specifically I don’t remember what the semantics for updating are. There was something subtle there. So maybe this isn’t a bug per se.

[gaearon]

I agree with you this looks weird though. Can you submit an isolated failing case PR for now that’s manually written?

And then add fuzz separately.

[gaearon]

I also opened #18669 for the broader fuzz question.

[dubzzz]

Can you submit an isolated failing case PR for now that’s manually written?

Yes, I will open a specific PR only containing a failing unit test related to this potential bug.

And then add fuzz separately.

And then open PRs to discuss fuzzing on SuspenseList. And link them to #18669.

[sebmarkbage]

Updates are indeed strange. Nesting is also strange.

What happens here is that we have two rules that can yield counter intuitive results:

• A row is not allowed to show content until the previous row is done.
• A row is not allowed to show partial content until the whole row is done.
• Never hide already shown content until it explicitly resuspends, to avoid content disappearing.

What's happening here is that the inner Suspense list is also a row. We follow the rule: "A row is not allowed to show partial content until the whole row is done."

It gets extra confusing because A is still showing due to the rule "Never hide already shown content until it explicitly resuspends, to avoid content disappearing".

So this behavior is "intentional" according to the rules.

We might have the wrong heuristics but a real world use case would be helpful to inform how they should change.

When you have two Suspense boundaries in a row that aren't wrapped in a SuspenseList there has to be kind of an implicit list there to decide how they should work with regard to each other. You wouldn't want the last one to pop in before the first one. So a good default is "A row is not allowed to show partial content until the whole row is done". It is as if the list was wrapped in SuspenseList revealOrder="together". However, I could imagine a rule like if all boundaries are in a single SuspenseList in a row, then that list decides.

[dubzzz]

@sebmarkbage
Thanks a lot for the explanation. Now I think I understand part of the behaviour I see:

• We see A in the second render because of the rule: Never hide already shown content until it explicitly resuspends, to avoid content disappearing

There is still something I don't fully get - Why is the behaviour different if I directly render:

render(
  <SuspenseList key="1" revealOrder="forwards">
    <SuspenseList key="1.1" revealOrder="forwards">
      <Suspense key="1.1.a" fallback={<div>Loading A</div>}>
        <A />
      </Suspense>
      <Suspense key="1.1.b" fallback={<div>Loading B</div>}>
        <B />
      </Suspense>
      <Suspense key="1.1.c" fallback={<div>Loading C</div>}>
        <C />
      </Suspense>
    </SuspenseList>
  </SuspenseList>
)

Case of the issue: https://codesandbox.io/s/mutable-rain-3ikor?file=/src/App.js (click on update)
Case with direct render: https://codesandbox.io/s/trusting-hoover-4cffd?file=/src/App.js

I mean according to your explanation I'd have expected to see Loading A / Loading B / Loading C but I see A / B / Loading C instead 🤔

(What is a row? A Suspense? A SuspenseList? Both? Something else?)

[gaearon]

I think a "row" is an immediate child of SuspenseList?

[sebmarkbage]

@dubzzz In your example I see A / Loading B / Loading C.

That's because we are not allowed to turn A back into "Loading A" because:
"Never hide already shown content until it explicitly resuspends, to avoid content disappearing."

We're not allowed to show "B" because:
"A row is not allowed to show partial content until the whole row is done.".
The row in this case is the whole list because it's a row of the outer list.

The solution btw is to wrap it in another Suspense boundary. Because that kind of "resets".

<SuspenseList key="1" revealOrder="forwards">
  <Suspense>
    <SuspenseList key="1.1" revealOrder="forwards">
      <Suspense key="1.1.a" fallback={<div>Loading A</div>}>
          <A />
      </Suspense>
      <Suspense key="1.1.b" fallback={<div>Loading B</div>}>
          <B />
      </Suspense>
      <Suspense key="1.1.c" fallback={<div>Loading C</div>}>
          <C />
      </Suspense>
    </SuspenseList>
  </Suspense>
</SuspenseList>

[dubzzz]

@sebmarkbage Thanks a lot for this very detailed explanation. I get a better idea of how updates are supposed to be working.

Maybe one last question: Do these rules only apply on updates? (not first render)

Indeed, they seem not to apply on first render. See the following example not behaving the same way?

https://codesandbox.io/s/trusting-hoover-4cffd?file=/src/App.js

In this second example I directly render: (with A, B already resolved BUT C still pending)

render(
  <SuspenseList key="1" revealOrder="forwards">
    <SuspenseList key="1.1" revealOrder="forwards">
      <Suspense key="1.1.a" fallback={<div>Loading A</div>}>
        <A />
      </Suspense>
      <Suspense key="1.1.b" fallback={<div>Loading B</div>}>
        <B />
      </Suspense>
      <Suspense key="1.1.c" fallback={<div>Loading C</div>}>
        <C />
      </Suspense>
    </SuspenseList>
  </SuspenseList>
)

So given your explanation I expect to have Loading A / Loading B / Loading C.

Indeed neither A, nor B have already been displayed so "Never hide already shown content until it explicitly resuspends, to avoid content disappearing." does not apply (first render).

Additionally as "A row is not allowed to show partial content until the whole row is done.", nothing from the row should be shown as C is still loading.

As a consequence of those two rules I expect to have Loading A / Loading B / Loading C but I get A / B / Loading C.

[sebmarkbage]

Yea that one is kind of surprising to me too.

Regardless I came across a case where we might want to change the rules like I mentioned above.

If you have a row that you want to consistently show in its loading state while popping in additional items one by one before it, then you need an inner list with tail=collapsed/hidden and an outer one which is not collapsed. But we force the last item not to show its fallback.

However if you do that then the inner one isn’t allowed to partially suspend.

So regardless I think we need to change this.

[hosseinmd]

What is the latest version of concurrent mode?