Root level suspension doesn't work as expected

[theninthsky]

Preact version: 10.10.2.

Bug Description

When wrapping the entire <App> with Suspense, we can see some odd behavior.

createRoot(document.getElementById('root')).render(
  <BrowserRouter>
    <Suspense>
        <App />
    </Suspense>
  </BrowserRouter>
)

It seems that only lazy chunks are suspended, instead of the entire app.

To Reproduce

Open the React version of the app: https://client-side-rendering.pages.dev
Open the Preact version of the app: https://8009dc66.client-side-rendering.pages.dev

In the network tab, block the index.[hash].js request URL for each page and reload (this blocked file is the Home page chunk).

The React app will look like this:

While the Preact app will look like this:

As we can see, the navigation bar does not suspend, since its not lazily loaded.

Source can be found here: https://github.com/theninthsky/client-side-rendering
There's a main (React) branch and a preact branch. Both are completely identical, except for the Preact configurations in webpack.config.js and tsconfig.json.

Expected behavior
The entire app should suspend until its lazy chucks are loaded.

Sorry to bother you again, but this is the final step towards completely replacing React with Preact.

[JoviDeCroock]

I haven't taken a look yet but if you wrap the children of your Suspense boundary with a <div> I assume it does work? Will take a look over the weekend probably

[theninthsky]

@JoviDeCroock
Unfortunately, that did not work either.

However, it fixed some initial layout shift issue I chose not to mention here (you can see that by refreshing the deployed Preact version of the app with cache disabled). So thanks for the tip!

[JoviDeCroock]

Oh, expected that to tackle both issues 😅 yes our Suspense implementation has some flaws at this moment in time especially with how it handles shifting the layout in and out.

First attempt at reproducing minimally https://codesandbox.io/s/practical-field-txgllk?file=/src/index.js

[JoviDeCroock]

One of your deps might be behaving weirdly in prod mode because I can't reproduce this in dev - checked out your repo and everything, it just works

Will need a minimal way to reproduce but generally would try with an empty div as fallback and see the results like that as it can give us a clue

[theninthsky]

@JoviDeCroock
Development mode (npm start) gives the same effect:

I tried replacing the Fragment with a div and it made the app get stuck upon navigation (but it resolved the layout shift glitch).

The new deploy (with the div) can be found here: https://e578fdf5.client-side-rendering.pages.dev

Edit: it seems that I've been missing on some parts - even before replacing the Fragment with div, my entire app freezes upon navigation.
It seems that 10.10.2 caused this new issue, and I just forgot to test it by navigating my app.
I tried moving Suspense to wrap only the Routes component but it made no difference.

10.10.0 [Issue] [Deploy]
10.10.1 [Issue] [Deploy]
10.10.2 [App freezes without throwing an error upon navigation] [Deploy]

Would you like me to open a separate issue for 10.10.2?

[JoviDeCroock]

I can reproduce the last thing you said i.e. a navigation resulting in some loop but the current one is not happening for me 😅 the navigation one doesn't seem caused by 10.10.2 in others the navigation just isn't working for i.e. 10.10.1 doing a hot fix there to not use useState causes that same issue

Screen.Recording.2022-08-11.at.08.36.19.mov

For the latter navigational bug it seems related to valtio, currently looking at their source code.

EDIT: it looks like #3655 has an effect on the useSyncExternalStore usage of valtio

[theninthsky]

@JoviDeCroock
You need to block the index.[hash].js in the devtools (or block the lorem-ipsum.[hash].js in the Lorem Ipsum page etc.) in order to see that the suspension doesn't work (the navbar "escapes" the suspension).

Thanks for trying to resolve these issues :)

[JoviDeCroock]

Oh you mean that when an error is thrown you prefer the UI not rendering in general?

[theninthsky]

@JoviDeCroock
Correct.
But the real intention behind this is to visually show the app in one draw:

https://github.com/theninthsky/client-side-rendering#preventing-sequenced-rendering

[JoviDeCroock]

Well just for clarity, it doesn't have much to do with showing the app in one draw at the moment as it will never show the part you are illustrating as your body as that is throwing an error. This is the resilience part of your application, recovering from errors, I would agree that maybe showing nothing when nothing is coming in the future is preferable but that would go paired with for instance using a loading fallback on a Suspense boundary.

We have done prior work on the whole swapping thing that I guess is illustrated under "sequenced" rendering, browsing around on wmr shows an early implementation of that where the boundary keeps around the prior page as a fallback while rendering the full new view when it comes in.

I deviate, I guess what we can do is destroy the UI when errors get thrown but yes error-boundaries are your friend there. Where we differ from React here is that we keep our VDOM and DOM around when a suspense boundary triggers because we don't want to reconstruct everything.

[theninthsky]

@JoviDeCroock
Oh I'm sorry, I didn't explain myself properly.
I don't expect there would be an error while fetching static assets from Cloudflare (it's extremely rare).

The only reason I block the url of the async pages is to demonstrate that that suspension doesn't work properly.
I just could not find another way to show it, since the page loads so fast.

However, I agree that suspending the entire app is not that important, since everything loads so fast that users will rarely see a "sequenced render".

Its initial intention was to make FCP and LCP equal.

[JoviDeCroock]

Saying this in the politest way possible, we are still talking about different things. I am saying that the suspension does work correctly try with

const wait = ms => new Promise(res => setTimeout(() => {
  res()
}, ms))
const Home = lazyPrefetch(async () => {
  await wait(2500)
  return import(/* webpackChunkName: "index" */ 'pages/Home')
})

Which will exhibit the aforementioned behavior correctly, as shown in the video. The difference is that if you block a request the JS-Runtime for import will throw an Error. Errors aren't handled by Suspense in our case as the behavior here slightly deviates from the default one.

[theninthsky]

@JoviDeCroock
Oh I see, so the method I chose to demonstrate the "lack of suspension" was the worst way possible (since React and Preact handle async chunks errors differently).

So in fact Preact's Suspense does work properly :)
Thanks for that!

I will close this issue now, would you like me to open an issue for the infinite loop when navigating?

[JoviDeCroock]

Feel free to open an issue, I have narrowed it down to zustand and the useSyncExternalStore of Preact not fully working with the selector usage of zustand https://codesandbox.io/s/great-microservice-i9p35o?file=/src/index.js @theninthsky