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

[React 19] Disabling prerendering siblings of suspended components breaking common pattern #29898

Open
ehellman opened this issue Jun 14, 2024 · 27 comments
Labels

Comments

@ehellman
Copy link
Contributor

Summary

I'm creating this issue to continue the discussion that spawned in the already merged PR (#26380)

Several community members have raised concerns about this change and it has gained traction on both X and Reddit as well, I think it is important to prioritize this discussion as it can be a dealbreaker whether or not a project will upgrade to React 19 at all of this is not addressed.

The problem

This change causes sibling components of suspended components to not prerender, meaning that it will cause performance problems when using a very common pattern where you do data fetching near to where the data is actually used. It makes requests that would before be performed in parallel due to prerendering triggering them, into waterfall requests instead. Here's screenshots from @matiasngf and part of their description from the pull request comment:

First, we recorded the current approach; it takes about 2.5 seconds to load the .glb models:

image

Then, we installed the canary version of react; Same .glb models, takes about 3.5 seconds to load:

image

I included the above example in the issue body because it is a great illustration of the problem. Their comment also include links to both their current site that doesn't have the problem and also a version of their site with an updated React with the problem present.

Discussions around potential fixes

There are several ways to approach this, but I think @matiasperz were spot on when suggesting that this entire change be opt-in. The changed logic seems like the part that should be opt-in, and the default behavior should be the way things worked before the change.

Something along the lines of:
strategy={"parallel" | "sequential"} where it is clear how suspended components will behave.

Conclusion

While not exactly a breaking change in the literal sense, I would personally consider this a breaking change and worse, one that it is not possible to easily migrate away from without changing the entire data fetching pattern for your application. The other option being just eating the performance loss but no one is realistically going to do that.

Moving data fetching up is not a good recommendation, since it goes against an extremely common and well-liked pattern in the community and the great thing about React has always been that we can choose our own patterns and develop amazing applications in several different ways. But changes like this, if you cannot opt-out of them (or in this case opt-in, because considering the effects it will have the sensible default should be the previous behavior) would break this very fundamental value of React.

Let's please continue to discuss this to find a good path forward.

@rickhanlonii
Copy link
Member

Thanks for submitting, let's use this issue to track the feedback for the suspense changes in 19.

@neil-morrison44
Copy link

neil-morrison44 commented Jun 14, 2024

Just to aid anyone looking for further discussion, there's a lot of great points being made by 0xca0a (@drcmda here) over on Twitter which I expect could gel with a lot of how us were viewing <Suspense> (especially if we've used react-three-fiber).

Particularly this & this about viewing Suspense as async task management (rather than strictly fetch).

Personally I've got code that Suspends when doing fully local async things like:

  • decoding binary data (and doing async blob stuff with it)
  • rendering to offscreen canvases
  • reading metadata from in-browser zip files
  • accessing IndexedDB, etc

which I was looking forward to migrating to use with React 19 but this change'll cause to waterfall without an opt in / out. Or breaking a nice separation of concerns by doing outside of where it gets (often conditionally) used.


I'll also restate my point from the PR comments that React really should have performance regression tests being run, instead of just bundle size. Since, assuming they were representative of React apps deployed in the wild, this would have been seen as a big change when it was proposed.

@sophiebits
Copy link
Collaborator

update fyi — just tweeted https://x.com/sophiebits/status/1801663976973209620

good news re Suspense, just met w/ @/rickhanlonii @/en_JS @/acdlite
* we care a lot about SPAs, team misjudged how many people rely on this today
* still recommend preloading but recognize not always practical
* we plan to hold the 19.0 release until we find a good fix

more to come

@ckifer
Copy link

ckifer commented Jun 14, 2024

Thank you and kudos to the team for the creation of this issue, listening to the concerns of the community, and the updates on this thread. Looking forward to the fix/discussions moving forward 🙌🏼

@LucaColonnello
Copy link

Just want to say, this is the perfect example of owning it! Thanks for considering the feedback of many!

@Nick-Lucas
Copy link

Nick-Lucas commented Jun 14, 2024

Initial explanation hits the nail on the head. Thank you for returning this to consideration!

If we can have a boundary-level opt-in or opt-out then I'm personally excited to be able to pick the mode based on the properties of the page/region I'm rendering. Sometimes prefetching is just the best choice, and then it makes total sense to use "sequential" and benefit from further performance gains.

So many great features in react can be adopted gradually and contextually, so this would be another great tool in that box to take a working app and enhance it

@drcmda
Copy link

drcmda commented Jun 15, 2024

Thank you React team, for listening!

Maybe this all stems from suspense being referred to as "data fetching" in the React docs, water falling after all is usually a browser concern. Vue is spot on with their docs, this is what i would wish for React as well:

Screenshot 2024-06-15 at 13 41 58

It would go a long way if suspense could be understood as compositional management of async task runners, independent of SSR, DOM, routes, fetch. It would make it much clearer why it cannot water fall (by default at least). Suspense is the only means to enable libraries and components to have true interoperability. I have wondered for years how it is not celebrated and used all over, and i think it might be communication.

Even the simplest constructs it enables are amazing. The following would have been impossible in React 15 for instance:

<Center>
  <SomeUIComponent />
  <SomeOtherUIComponent />  
</Center>

Because how could <Center> know if a child is async or has finished its task runners, be it fetch, wasm, workers et al. Thanks to suspense it can safely assume that all children are completed come useLayoutEffect or useEffect, and act upon the results.

I wish i could some day show you how pmndrs has built eco systems on top of it. It seems forms-React has only just noticed it can fetch in-place and access results without useEffect + setState, but there is so much more to suspense than that.

@krispya
Copy link

krispya commented Jun 15, 2024

It has already been said very well how we have many async tasks that have nothing to do with data fetching in the pmndrs ecosystem. I just want to add that async being built into Javascript is one of its major strengths and while we are encountering this friction with our 3D libraries currently, it will not be limited to that. For example, any AI library will need async for everything it does, from setting up its program with the GPU to sending and receiving messages. React 19 needs to handle bundles of async tasks generally or it will not be compatible with general JS features going forward.

@revskill10
Copy link

Thank you React team, for listening!

Maybe this all stems from suspense being referred to as "data fetching" in the React docs, water falling after all is usually a browser concern. Vue is spot on with their docs, this is what i would wish for React as well:

Screenshot 2024-06-15 at 13 41 58 It would go a long way if suspense could be understood as compositional management of async task runners, independent of SSR, DOM, routes, fetch. It would make it much clearer why it cannot water fall (by default at least). Suspense is the only means to enable libraries and components to have true interoperability. I have wondered for years how it is not celebrated and used all over, and i think it might be communication.

Even the simplest constructs it enables are amazing. The following would have been impossible in React 15 for instance:

<Center>
  <SomeUIComponent />
  <SomeOtherUIComponent />  
</Center>

Because how could <Center> know if a child is async or has finished its task runners, be it fetch, wasm, workers et al. Thanks to suspense it can safely assume that all children are completed come useLayoutEffect or useEffect, and act upon the results.

I wish i could some day show you how pmndrs has built eco systems on top of it. It seems forms-React has only just noticed it can fetch in-place and access results without useEffect + setState, but there is so much more to suspense than that.

Sorry but i'm a bit confused on this.
Generally, if i understand your point, there should be 2 kinds of Suspense. The headless Suspense and the Rendering Suspense, which acts differently.

Using Suspense currently promotes the use of render-then-fetch strategy, which is bad on slow-network condition.

@pkellner
Copy link

Any update on when this issue may be resolved and how it will impact the R19 release schedule?

@eps1lon
Copy link
Collaborator

eps1lon commented Jun 24, 2024

Any update on when this issue may be resolved and how it will impact the R19 release schedule?

#29898 (comment) is still the latest.

@alexreardon
Copy link

alexreardon commented Jun 25, 2024

Thanks so much for your hard work in this difficult domain!

Out of interest, I have explored what a <Suspense> algorithm would look like if all thrown promises were resolved before trying to re-render ('wait for pending').

→ Write up

TLDR: 'Wait for pending' is an interesting variation of the react@18 ("parallel") and react@19 (alpha) ("serial") <Suspense> algorithms. For flat async trees, 'wait for pending' allows for fast calling of render functions, and minimal re-renders. For trees with nested async components, child async components will have their initial render called slower than the react@18 algorithm.

I am not suggesting that 'wait for pending' is used, but it is interesting and might spark other ideas. My thinking right now is that providing an option for <Suspense> (eg "serial" or "parallel") would be a great path forward.

@IVLIU
Copy link

IVLIU commented Jun 27, 2024

I've reviewed the original PR. Regarding the issue with use, could we maintain a ThenableList on the Suspense fiber? We would always throw the first unresolved promise until it is resolved, then look for the next one until we reach null.

@IVLIU
Copy link

IVLIU commented Jun 28, 2024

or like this

<Suspense>
  {(isSuspend) => (
    <></>
  )}
</Suspense>

even though I really hate it, I also don't want to do it this way.

@ehellman
Copy link
Contributor Author

ehellman commented Jul 2, 2024

@rickhanlonii @sophiebits Thanks for picking this up. It's a bit sad that this issue got a lot of traction on Twitter with many interesting discussions but hardly anyone of these people have contributed to the discussion here, where the code is, where the change will take place and where people can search for these discussions in the future to ensure we don't repeat ourselves or to learn about previous decisions.

That said, it would've been a much bigger problem if this was a more complex issue where it felt uncertain what the end goal should be. Right now it feels like both the part of the community that reacted to this change and the team are both clear on what the end goal is and how we want this feature to behave. It's just worth to think about in the future, ensuring that we bring important information back close to the code.

@mattbolt
Copy link

Right now it feels like both the part of the community that reacted to this change and the team are both clear on what the end goal is and how we want this feature to behave.

I've spent hours pouring over online commentary on this topic and I'm struggling to find what the final consensus was for resolving this, and what kind of timeframe we have until React 19 is stable (as most places are singularly blaming this topic for the hold up).

Is there any updates and indications when it will be resolved?

@mikegreiling
Copy link

Is there any WIP that one can follow along with here? Are discussions happening internally with the React core team, or is there a public discussion or PR somewhere to address this?

@eps1lon
Copy link
Collaborator

eps1lon commented Aug 5, 2024

It's still being worked on. Since the original change was part of the Canary release for so long, a lot of changes were built on top of it. Changing this behavior affects a lot of scenarios that we need to carefully examine to be sure we don't regress in scenarios that should remain untouched. This takes a while to get right.

You can subscribe to this issue and will get a notification from GitHub once the fix has landed.

@HarshAggarwals
Copy link

Hi @ehellman @eps1lon, instead of having parameter, strategy={"parallel" | "sequential"}, we can have param like runInParallel={true | false}. It will be much easy to understand, and also does not require to remember "parallel" & "sequential" texts. Also passing texts, can lead to spelling mistakes.

@Ayc0
Copy link

Ayc0 commented Aug 25, 2024

@HarshAggarwals you have the same issues with a boolean: "parallel" is written in the property name so you still need to remember it. And you can make typos in the prop name, and of you're using types (with flow or TS), you don't have to remember this

And using strings allows more strategy in the future (so more flexible than a boolean)

@markerikson
Copy link
Contributor

As an FYI, Andrew just put up the first PR that looks to implement actual changes related to parallel pre-rendering, here:

@oemer-aran
Copy link

@markerikson Super dope! Does this mean react 19 will be stable soon? Or are there any other blockers?

@markerikson
Copy link
Contributor

@oemer-aran I'm the wrong person to ask about this :)

I've seen additional PRs that look possibly related to this.

For example, this refactoring PR:

was followed by another "prerendering" PR that then got reverted:

I don't have any inside information here, I just keep an eye on the PRs.

@LZL0
Copy link

LZL0 commented Nov 13, 2024

#31452 seems to do the job.
Merged into Next with vercel/next.js#72768.

@rickhanlonii
Copy link
Member

rickhanlonii commented Nov 14, 2024

Update: New in React 19 - Pre-warming

Hey all, thanks for being patient while worked on a fix for this. We just released [email protected] which includes a new feature to address this. We think this feature is the best of both worlds - suspense fallbacks will still commit immediately, but we'll also render the siblings of suspended components to "pre-warm" the lazy requests to prevent waterfalls when fetches are not preloaded.

tl;dr: React 19 Upgrade Guide - Improvements to Suspense.

Details

In React <=18, when a component suspended we would continue rendering the rest of the siblings before committing the fallback to the user. This was good for initializing lazy fetches and preventing waterfalls, but bad for the user because it delayed the critical time to show the fallback, and if you pre-loaded requests (recommended) this delay was unnecessary:

1

In React 19 RC0, we stopped rendering the siblings so that the fallback could be committed immediately. This allowed the fallback to be displayed faster, which benefitted the recommended use case of preloading requests, because there was no wasted work:

2

As this issue notes, this change meant lazy requests initiated at render time would be forced to waterfall, and as @sophiebits explained, we underestimated the prevalence of having a single Suspense boundary containing siblings that each initiate different async work — since you don't run into this if you initiate fetches earlier or if things are in one component.

To address the issue, RC1 adds a new feature called prewarming. With prewarming, React will still immediately commit the fallback when a component suspends, but we will schedule another render for the siblings in order to "prewarm" the lazy requests in that tree while the tree is still suspended:

3

This means we're able to get the benefits of committing the fallback immediately, without the downsides of creating waterfalls for lazy requests.

Notes

  • This feature is implemented for both use and for the legacy promise throwing behavior.
  • We included infinite loop protection so that apps don't get stuck in a suspend -> pre-warm -> suspend loop.
  • We do not pre-warm when suspending during hydration. Adding this for hydration is particularly tricky to implement, since suspending during hydration is already a deopt to client render with a warning to fix, we don't think it's worth blocking 19 on further.

Next steps

Please install RC1 and try it out. We've tested this across Meta and saw no regressions.

Pending feedback, we hope to publish 19 stable soon.

Thanks

Huge thanks to @sebmarkbage, and @acdlite for designing and implementing this feature, @sophiebits for feedback on the design, and @jackpope for experimenting with it at Meta and helping to debug any issues we found. @acdlite especially put a ton of work over the last few months to implement this and handle all the edge cases:

@guillaumebrunerie
Copy link

@rickhanlonii The "Other notable changes" section in https://react.dev/blog/2024/04/25/react-19-upgrade-guide#other-notable-changes still contains "Don’t prerender siblings of suspended component #26380". I guess it should be removed?

@elitalpa
Copy link

This solution seems great !

Commiting the fallback immediately is better than the past behavior, and the prewarming feature is a good fix and addition.
Many thanks for all the work that went into this and for considering the feedback.

Looking forward to the React 19 stable release.

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

No branches or pull requests