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

RFC: Suspense in React 18 #213

Merged
merged 8 commits into from
Mar 28, 2022
Merged

RFC: Suspense in React 18 #213

merged 8 commits into from
Mar 28, 2022

Conversation

gaearon
Copy link
Member

@gaearon gaearon commented Mar 23, 2022

In this RFC, we describe a few changes to Suspense that we intend to ship in React 18.

View the formatted RFC.

@devknoll
Copy link

One issue that I saw with Next.js is that some folks are already using Suspense on the client, and such APIs might not work well with server rendering without additional changes.

Would it make sense to officially recommend something like having existing client-only suspense APIs start throwing an error when rendered on the server so that they fallback to client-rendering, until the API can be updated to properly support server-side Suspense?

text/0000-suspense-in-react-18.md Outdated Show resolved Hide resolved
text/0000-suspense-in-react-18.md Outdated Show resolved Hide resolved
gaearon and others added 2 commits March 23, 2022 19:58
@gaearon
Copy link
Member Author

gaearon commented Mar 23, 2022

Re: #213 (comment)

One issue that I saw with Next.js is that some folks are already using Suspense on the client, and such APIs might not work well with server rendering without additional changes.

Is it the case where people do conditional rendering like {typeof window !== 'undefined' && <Suspense>...</Suspense>}, or is this some other case? Currently, it throws on the server if rendered unconditionally. Rendering different content on the client and server based on a condition was never officially supported, so I'm not sure which scenario this refers to.

If it's the scenario I'm thinking of, the fix is usually to do rendering in two passes:

const [mounted, setMounted] = useState(false);
useEffect(() => {
  setMounted(true);
}, []);

<div>
  {mounted && <Suspense>...</Suspense>}
</div>

It's not great but would have the same behavior.

Would it make sense to officially recommend something like having existing client-only suspense APIs start throwing an error when rendered on the server so that they fallback to client-rendering, until the API can be updated to properly support server-side Suspense?

I'm not sure I understand what you're proposing. Is it to make renderToString throw on Suspense nodes? Or something else? Thanks!

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Mar 24, 2022

@gaearon The case is not something that happens by just upgrading to newer React. It's when you first upgrade to newer React and then later start using existing Suspense-y APIs on the server and <Suspense> "just works". Except it doesn't just work because the result isn't serialized in the stream so it might change and cause hydration errors.

So there's no conditional Suspense.

IMO, it's correct that an API that tries to be suspensey but doesn't have a way to transfer data to the client should throw an error if used on the server so that it deopts to client rendering and you get the correct client side warning.

Typically such an API has some way to provide initial data from getServerSideProps/getInitialProps. So it can throw only if that wasn't provided

@gaearon
Copy link
Member Author

gaearon commented Mar 24, 2022

existing Suspense-y APIs on the server

Do you mean third-party libs that throw Promises today?

@gaearon
Copy link
Member Author

gaearon commented Mar 24, 2022

OK yeah got it. I thought this was about using <Suspense> for code splitting, but I see now the question was about using libraries that try to use Suspense for data fetching (which is not fully supported yet), and the fact that these libraries used to throw on the server, but now they may just subtly be broken. I agree it makes sense for them to throw explicitly until there is a way to transfer the data to the client.


The reason we didn't go with this approach in React 16.6 was for backwards compatibility reasons. At the time, most React code used classes, and many classes contained the `componentWillMount` method. This is why we renamed it to `UNSAFE_componentWillMount` in 2018 and [described](https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html) strategies ot migrate away from it. The problem with `UNSAFE_componentWillMount` is that it fires during rendering (so, before we know whether child components suspended or not). If we throw away an incomplete tree after `UNSAFE_componentWillMount` has already fired, it will not receive a matching `componentDidMount` or `componentWillUnmount` call. (During a retry, there will be another `UNSAFE_componentWillMount` call because we need to render the same tree again.) So code that relies on `UNSAFE_componentWillMount` and `componentWillUnmount` being called the same number of times might cause mistakes or memory leaks.

We don't think this concern is relevant anymore for several reasons. `UNSAFE_componentWillMount` was marked as "unsafe" in 2018, and most popular open source libraries have long migrated away from it. This issue also only affects components "between" the `<Suspense>` node and the component that actually suspends. So it is very local in scope, and is easy to fix. Finally, Hooks have become a popular alternative to classes, and don't have the same issue. On the other hand, the current behavior [has been causing issues](https:/facebook/react/issues/14536) for using popular component libraries with Suspense. This is why we think now is a good idea to make that change.
Copy link

Choose a reason for hiding this comment

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

Is there a timeline for when these UNSAFE_ methods would go away? This seems to be the first concrete reason to move away from these APIs.

Choose a reason for hiding this comment

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

FYI there’s a typo on line 123, after the link for [described]:
ot should be to
strategies ot migrate -> strategies to migrate
Love all this btw!

Copy link
Member Author

@gaearon gaearon Mar 24, 2022

Choose a reason for hiding this comment

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

For what it’s worth, componentWillMount potentially firing multiple times was the original reason that it was renamed to UNSAFE in 2018. So I wouldn’t quite say that it’s “the first concrete reason” now. It’s what the reason has been along from the beginning. We didn’t anticipate that the whole journey to actually taking advantage of this would take so long, but this reason was exactly why we suggested to move away from it back then.

There is currently no plan to “remove” the UNSAFE methods. They’ll keep firing. It’s just that if you use Suspense, UNSAFE_componentWillMount may potentially execute multiple times without corresponding DidMount/WillUnmount if it’s in a component between Suspense and the part that suspends. Since many old componentWillMount code is not resilient to that, “UNSAFE” helps highlight that this code can be a problem.

Copy link

Choose a reason for hiding this comment

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

Gotcha, this to me just feels like the first step towards a world without these UNSAFE methods being available at all (with the language of its been 4 years and libraries have moved away etc) so I was curious if there was a timeline for that.

For context at my company we have > 500 instances of just UNSAFE_componentWillMount and are currently undergoing a migration to move away from all UNSAFE methods. But overall its always easier to prioritize such work when they are being removed and a hard blocker for the upgrade vs potentially subtle bugs / opt into R17 behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. Our internal policy was to forbid them in any new code (or any code imported into new code) by using Strict Mode which complains about them. And then for old code (of which there is plenty), the policy is to exercise caution before using new features like Suspense. So it could still be added there but there are possible new memory leaks etc.

@voluntadpear
Copy link

Are the layout effects to re-run when content reappears effectively only those defined using useLayoutEffect? Although not recommended due to visual inconsistency, DOM mutations and reading using useEffect also happen (folks sometimes move to useLayoutEffect only after noticing blinks, layout shift, etc.) Are those effects also re-running?

@gaearon
Copy link
Member Author

gaearon commented Mar 24, 2022

Re: #213 (comment)

Are the layout effects to re-run when content reappears effectively only those defined using useLayoutEffect?

Yes, as well as ref callbacks.

DOM mutations and reading using useEffect also happen (folks sometimes move to useLayoutEffect only after noticing blinks, layout shift, etc.) Are those effects also re-running?

No, useEffect does not re-run. If it's visually important then useLayoutEffect is the right tool to use, so this builds on the existing distinction (like you said, otherwise you might have shifts anyway). Not re-running useEffect is important because it leaves you a workaround for things that should not reset. It also avoids performance issues with running too much setup/cleanup code when hiding/showing stuff.

Edit: I discussed with the team a bit. useEffect indeed does not re-run currently in this particular scenario. However, in the future major releases it's possible we'll disconnect effects too, like if something suspends for a long time. There are other features like Offscreen that always disconnect all types of effects on hide/shown because the cost of that is often lower than the cost of maintaining subscriptions etc. So for now we're starting with useLayoutEffect but it might expand.

@bennettdams
Copy link

bennettdams commented Mar 24, 2022

This sounds trivial, but: What are the current scenarios that allow a component to suspend in general?

The whole RFC gives explanations for what happens when a component suspends, but how do I, as a developer, enable a component to suspend? With that I mean throwing a promise, startTransition, and so on.
Or, in other words: What are the "tools" we can use in our code so <Suspense> (and its fallback) is actually used?

I saw "The exact protocol a component should use to tell React that it's suspended." at the "Unresolved questions" - maybe I'm lost in translation, but I read this as "How a component lets React know while it is suspending" (=> "loading state").
Sorry for asking if it was not meant like this.

@gaearon
Copy link
Member Author

gaearon commented Mar 24, 2022

Re: #213 (comment)

The whole RFC gives explanations for what happens when a component suspends, but how do I, as a developer, enable a component to suspend?

Indeed, that's the "unresolved question" you're mentioning. The pragmatic answer is that today it works by throwing a Promise. However that's not an "official" or "final" API, and it's also not a complete description of the contract. You can't just throw arbitrary promises and expect it to work. Rather, it has to be backed by some cache outside of your components (rather than create during rendering), you need to throw the same Promise while it's pending (rather than creating a new one every time), you have to store its result outside of your components, you can't mutate the received data, and so on. These requirements are somewhat difficult to satisfy on your own, which is why we don't consider Suspense for data fetching officially supported yet. reactwg/react-18#25 is part of the work to make this simpler.

@IDrissAitHafid
Copy link

I have a question, with this new behavior, if Panel is a big component that takes time to render it's better to put it outside the fallback like this:

<Panel>
  <Suspense fallback={<Spinner />}>
    <Comments />
  </Suspense>
</Panel>

Because with the previous behavior Panel is already mounted, thus we don't have to wait for it to mount again once the Comments times out?

However, I am more in favor of the new approach because, in addition to its simplicity and all the reasons you have mentioned, I think it also simplifies react-devtools code by removing many of the Suspense edge cases, thanks to the fact that Suspense now behaves the same as regular fibers from the devtools perspective, right?

@gaearon
Copy link
Member Author

gaearon commented Mar 24, 2022

Re: #213 (comment)

Because with the previous behavior Panel is already mounted, thus we don't have to wait for it to mount again once the Comments times out?

Re-rendering always starts from the Suspense node, so even in the old behavior, Panel would get re-rendered when Comments completes.

I think it also simplifies react-devtools code by removing many of the Suspense edge cases, thanks to the fact that Suspense now behaves the same as regular fibers from the devtools perspective, right?

I'm not familiar enough with React DevTools internals to answer that.

@wintercounter
Copy link

I recently had proplems with Suspense during hydration. I had a text mismatch warning and then that immediatelly failed the Suspense root and switched to client side rendering. If it's "just" a warning, I wouldn't expect the whole thing to break. If it's an error, why is it only a warning? :) I tested with suppressHydrationWarning just out of curiousity, and my app didn't switch to client render anymore. Is this really the expected behavior?

@gaearon
Copy link
Member Author

gaearon commented Mar 25, 2022

Re: #213 (comment)

Yes, we’re treating hydration mismatches as errors now. They cause a fully client render at the granularity of the closest Suspense boundary above the mismatch. I will post more details about this later today. The correct fix is usually to fix the mismatch but you’re right that for things like timestamps, the “suppress warning” prop now actually has production behavior too. This is unfortunate and we might rename it in a future release.

@sebmarkbage
Copy link
Collaborator

We've effectively introduced a new form of Error that's kind of between a Warning and an Error... "Recoverable Error". It just means it can recover by client rendering so the user won't see it but it's an error so worth fixing.

@gaearon
Copy link
Member Author

gaearon commented Mar 25, 2022

More on that: #215

gaearon added a commit that referenced this pull request Mar 28, 2022
* RFC: Intent to ship React 18

This RFC describes the changes we intend to ship in React 18.

* Add link to RFC #213

* Move the note up

* Add RFC link

* Update text/0000-react-18.md

Co-authored-by: Sebastian Silbermann <[email protected]>

* Add useDeferredValue

* Update and rename 0000-react-18.md to 0212-react-18.md

Co-authored-by: dan <[email protected]>
Co-authored-by: Sebastian Silbermann <[email protected]>
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.