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

Integrating Remix Router with React Transitions #5763

Closed
1 task done
gaearon opened this issue Mar 10, 2023 · 16 comments
Closed
1 task done

Integrating Remix Router with React Transitions #5763

gaearon opened this issue Mar 10, 2023 · 16 comments
Labels
enhancement New feature or request feat:routing

Comments

@gaearon
Copy link

gaearon commented Mar 10, 2023

What version of Remix are you using?

1.14.1

Are all your remix dependencies & dev-dependencies using the same version?

  • Yes

Steps to Reproduce

Remix does not fully integrate with Suspense because the Remix router does not use startTransition. I was wondering if there is interest in exploring a fuller integration where Remix router would be more Suspense-y.

To illustrate this, I will offer a little example.

Initial setup

Suppose I have an /index route that just renders <h1> and a more complex /about page that loads some data. My navigation to /about looks like this:

remix_good.mov

The data loads in two stages:

  1. First, the "blocking" part holds the router navigation back.
  2. Then, when the loader is ready, we show some content and "defer" the rest.

My code for about.js looks like this:

import { Suspense } from 'react'
import { Await, useLoaderData } from "@remix-run/react";
import { defer } from "@remix-run/node";

function sleep(ms) {
  return new Promise(resolve => setTimeout(resolve, ms))
}

export async function loader({ request }) {
  await sleep(1000);
  return defer({
    username: 'taylor',
    more: sleep(3000).then(() => ({ bio: 'Hi there!' }))
  })
}

export default function Index() {
  const data = useLoaderData();

  return (
    <>
      <h1>About {data.username}</h1>
      <Suspense fallback={<h2>Loading bio...</h2>}>
        <Await resolve={data.more}>
          {more => <p>{more.bio}</p>}
        </Await>
      </Suspense>
    </>
  );
}

So far so good.

A wild suspense appears!

Now let's say I add a component that suspends to my /about page.

import { lazy, Suspense } from 'react'
import { Await, Link, useLoaderData,  } from "@remix-run/react";
import { defer } from "@remix-run/node"; // or cloudflare/deno
+ import Counter from '../counter'
...
export default function Index() {
  const data = useLoaderData();

  return (
    <div>
      <h1>about {data.username}</h1>
+     <Counter />
      <Suspense fallback={<h2>Loading bio...</h2>}>
        <Await resolve={data.more}>
          {more => <p>{more.bio}</p>}
        </Await>
      </Suspense>
    </div>
  );
}

Suppose that Counter itself suspends for whatever reason. E.g. maybe it renders a lazy component inside. Maybe it does a client fetch with one of the Suspensey libraries. Maybe it is waiting for some CSS chunk to load (when we add Suspensey CSS integration into React). Etc. In general, you can expect components anywhere in the tree to be able to suspend — and that includes the "non-deferred" parts of the UI.

At first, we'll have an error:

Screenshot 2023-03-10 at 20 39 47

OK, fair enough, it's a React error. It says there's no <Suspense> above it in the tree. Let's fix this.

I could add it around my outlet:

+        <Suspense fallback={<h1 style={{ color: 'red'}}>loading</h1>}>
            <Outlet />
+        </Suspense>

But then the whole page "disappears" when it suspends:

remix_flash1.mov

Or I could add it around the <Counter> itself:

      <h1>about {data.username}</h1>
+     <Suspense fallback={<h1 style={{ color: 'red'}}>loading</h1>}>
          <Counter />
+     </Suspense>
      <Suspense fallback={<h2>Loading bio...</h2>}>

But then my Counter "pops in" independently from the <h1>.

remix_flash2.mov

What I really want is to delay the route transition until all content in the "blocking" part (including <Counter /> and whatever it suspended on) is ready.

Is this even possible to fix?

Yes. The canonical fix for this is to wrap the setState which caused the navigation into startTransition. This will tell React that you don't expect that setState to finish immediately, and so it can keep showing the "previous" UI until the render caused by that setState has enough data to show meaningful UI.

As a concrete example, here is a similar navigation implemented in Next.js 13 (App Router):

next_good.mov

Note that although <Counter /> suspends on the client (with an artificial delay), the router still "waits" for it — it "pops in" together with the <h1>. I did not need to add any <Suspense> boundaries — neither around it nor above it.

import { Suspense } from "react";
import Counter from '../counter'

function sleep(ms) {
  return new Promise((resolve) => setTimeout(resolve, ms));
}

async function loadUsername()  {
  await sleep(1000);
  return "taylor";
}

async function loadBio() {
  await sleep(3000);
  return "hi there";
}

export default async function About() {
  const bioPromise = loadBio();
  const username = await loadUsername();
  return (
    <>
      <h1>about {username}</h1>
      <Counter   />
      <Suspense fallback={<h2>Loading bio...</h2>}>
        <Bio data={bioPromise} />
      </Suspense>
    </>
  );
}

async function Bio({ data }) {
  const bio = await data;
  return <p>{bio}</p>;
}

Of course, if I wanted to, I could add a <Suspense> boundary around just the <Counter>. Then it would not "hold" the router from a transition.

What's the higher-level point?

Remix currently implements its own machinery to "hold things back" until loaders have returned their result. Then it immediately "commits" the UI — even if that UI might immediately suspend.

React implements a first-class feature to "hold things back" — startTransition. (Or useTransition so you can report progress.) So I am wondering if there is some path towards using the built-in feature.

I imagine that, if Remix were to do that, it would start rendering the new route immediately — but it would suspend when in useLoaderData. If the router setState is wrapped in startTransition, you'd automatically get the "stay on the previous route until there's enough data to show the next route's shell" behavior. But unlike now, it would be integrated with anything else that suspends, like <Counter />.

Would love to hear any concerns or if there's something missing in React to make this happen. It's exciting Remix is taking advantage of Suspense for React SSR streaming already. Integrating Remix Router with React Transitions seems like a big next step towards Remix users being able to benefit from all React features.

Expected Behavior

  • Remix router transitions are React Transitions (see above)

Actual Behavior

  • Remix router transitions are not React Transitions (see above)
@ryanflorence
Copy link
Member

ryanflorence commented Mar 10, 2023

We started Remix on Suspense with the experimental releases back in the day but after a couple releases broke things we stopped relying on it to "hold things back" and stuck to the stable React releases.

Fast forward and now React Router uses useSyncExternalStore (https://github.com/remix-run/react-router/blob/main/packages/react-router/lib/components.tsx#L63-L70). In Remix v2 we're cleaning up the integration points with React Router and I imagine we won't have our own setState at all in Remix at that point, so it'll all just be useSyncExternalStore.

In that world do we still need startTransition?

@gaearon
Copy link
Author

gaearon commented Mar 10, 2023

and stuck to the stable React releases.

Fair enough! I think everything that's needed here is available in React 18, except for the "throw Promise" thing not being official (which would later become official "use"). But setting that aside, this should be possible to build with stable releases (e.g. it's how Relay works).

In Remix v2 we're cleaning up the integration points with React Router and I imagine we won't have our own setState at all in Remix at that point, so it'll all just be useSyncExternalStore.

At the moment, we can't make Transitions work with external stores. (Hence "sync".) I think we've explored potential APIs for more sophisticated multi-version stores but it's further out.

For Transitions to work, the state needs to be "rooted" in React (i.e. useState or useReducer) because we need to be able to pass different versions of state (either current or pending) to your component. This is what allows us to start rendering the "next' version (which may suspend) but keep responding to interactions with the previous version. I believe in Next 13, it's implemented as useReducer in the router component.

In that world do we still need startTransition?

If you'd like Remix users to be able to use Suspense fully (and avoid the issues described above), then yes.

I think it's also reasonable that Remix may choose not to support these patterns, and maintain its parallel implementation. I.e. not every React framework has to support the full range of React features.

Supporting only a subset of React features makes sense if the framework needs to cater to several UI libraries. Then it would have to use the lowest common denominator and, in the current landscape, it's sync rendering.

@ryanflorence
Copy link
Member

ryanflorence commented Mar 10, 2023

If you'd like Remix users to be able to use Suspense fully

Remix is designed specifically to avoid this kind of UX where render side effects trigger asynchrony, slowing down the page shifting the layout all around the screen as placeholders popcorn in.

Would love a concrete example of why you'd want to encourage these waterfalls when Remix has patterns to avoid it completely?

I'm quite happy with defer + <Suspense>, what else do you have in mind?

@gaearon
Copy link
Author

gaearon commented Mar 10, 2023

shifting the layout all around the screen

In the above example, it's the Remix version that shifts layout (because you're forced to add a boundary and can't let the content block instead). In the Next example, there is no extra shift because anything that suspends at the top-level "joins" the transition and blocks it. With Next, you have the freedom to decide what should block and what shouldn't. But you decide that on the UI level (with Suspense), not on the model layer (like defer).

If we set aside implementation concerns and forget about frameworks, I think “stuff outside <Suspense> blocks the route, but stuff inside <Suspense> doesn’t” is a rather intuitive heuristic.

where render side effects trigger fetches

Suspending doesn't mean a fetch is necessarily triggered on the client or by rendering. It only means that the component is not ready yet. It could be waiting for something that was triggered earlier and is already being fetched or transferred from the server. (From what I understand, this is how defer already works.)

For example, we're working on a CSS integration so that CSS chunks can be sent in parallel with JS (instead of blocking JS execution on them or loading them all upfront). If JS loads before associated CSS, React can start rendering the component. (Yay, no CSS-JS waterfalls!) But React would "wait" for associated CSS to load before revealing the contents of the Suspense boundary whose content depends on that CSS.

Images is another example where we might offer an API to "hold back" the boundary so that the route "pops together" with, say, a blurred cover image, or button icons.

Another use case is loading code — but it's true Remix is already limited in that sense. From what I understand, Remix can only load code eagerly (bundled with the route) or completely lazily (requested after client render attempt). So there is no way to conditionally prefetch Counter code based on whether loader's data says it will need to be rendered. However, if Remix did support that (via RSC or some other mechanism), then that would be another example of suspending on something that was already being fetched. Like defer but for code.

Finally, some things might just have to fetch on the client. I think it would be great if when you're forced to do this, you could still avoid unintentional layout jumps, and could hold back the router a bit.

All in all, I don't think this is strictly about one specific pattern. React offers a way to hold the UI back. It's ok not to use it, but I think it will start being more noticeable as more things start integrating it. Both under-the-hood things like CSS, and arbitrary user code taking advantage of this support.

@ryanflorence
Copy link
Member

I think it would be great if when you're forced to do this, you could still avoid unintentional layout jumps, and could hold back the router a bit.

That makes sense. I've wanted the ability to hold back the router to complete an animation in case the route transition is faster than the animation, or simply avoid flickering a spinner for 10ms.

I still maintain that we should be spending our energy designing APIs to avoid render-initiated asynchrony. Async components make it easier to do the wrong thing.

@tom-sherman
Copy link

tom-sherman commented Mar 11, 2023

I feel like the async/await and server components talk is derailing this issue a bit. It doesn't seem exactly related to the discussion of "remix router transitions should be wrapped in startTransition" - I think the code in the issue description is just showing how it works in server components, but it also applies to the remix of today when you start adding in suspense in it's many forms outside of defer().

@gaearon
Copy link
Author

gaearon commented Mar 11, 2023

Yeah sorry, didn’t mean to imply this has something to do with RSC per se. It’s just the latest example of “stuff can suspend anywhere” in my mind because it’s more granular about sending code. But the CSS stuff I mentioned is not gonna be RSC-specific — I think it would actually be great if Remix could try using it too once we get something working for that.

I also apologize if the focus on that Next example is distracting — Next isn’t where this behavior is. The heuristic itself is in React 18. (Transitions don’t get committed until you have enough to render the new screen.) So you can imagine the existing useLoaderData API, but if the router is implemented in the Suspensey paradigm (rendering the next route is a transition whose render suspends on its data/code), it should “just work” using the same heuristic.

@Moro-Code
Copy link

Forgive me for intruding on this conversation considering I'm a nobody 😅. @gaearon with the example you gave with the Counter component, let's say for whatever reason it takes 30 seconds to resolve. Maybe its calling some long running external process and we have no control on how long it takes. With the startTransition approach wouldn't the user basically be stuck on the previous page until whatever long running process Count called completed ?

You could certainly wrap it in Suspense and show the new page immediately with a fallback but that would need to be something intentional a developer does. If a developer misses that a component will not resolve in a reasonable amount of time well then you have a more jarring user experience in that the user thinks the site is stuck as opposed to showing the new page immediately and then the loader.

It would be great if startTransition maybe had something like a timeout that can be configured. Perhaps that could incentivize adoption as in the very least if a developer makes a mistake or doesn't anticipate something then there is a safety net ?

@tom-sherman
Copy link

What you're describing is what the browser already does natively on anchor tag, full page reload navigation. It makes sense for Remix to mirror this as a default IMO.

@gaearon
Copy link
Author

gaearon commented Mar 18, 2023

For example, we're working on a CSS integration so that CSS chunks can be sent in parallel with JS (instead of blocking JS execution on them or loading them all upfront). If JS loads before associated CSS, React can start rendering the component. (Yay, no CSS-JS waterfalls!) But React would "wait" for associated CSS to load before revealing the contents of the Suspense boundary whose content depends on that CSS.

Images is another example where we might offer an API to "hold back" the boundary so that the route "pops together" with, say, a blurred cover image, or button icons.

To put something concrete behind this, we've just landed core support for this in the reconciler (facebook/react#26398). No DOM integration yet, but it's coming. This will — at least initially — only be enabled during Transitions.

@kof
Copy link

kof commented Mar 21, 2023

Here is what I understood from reading this issue:

  1. React 18 has a new primitive (transitions)
  2. Transitions allow synchronizing async dependencies (components, CSS, data)
  3. Once transitions reach major adoption, the ecosystem will start relying on them
  4. If Remix doesn't adopt transitions support at least as an optional mechanism - it will become incompatible with the ecosystem that relies on transitions.

@damianstasik
Copy link

@Moro-Code for some time you could pass an object with options to useTransition and control the timeout with the timeoutMs property. Here is the explanation why it was removed: facebook/react#19703

https://hu.reactjs.org/docs/concurrent-mode-patterns.html#transitions

@gaearon
Copy link
Author

gaearon commented Mar 30, 2023

with the example you gave with the Counter component, let's say for whatever reason it takes 30 seconds to resolve. Maybe its calling some long running external process and we have no control on how long it takes. With the startTransition approach wouldn't the user basically be stuck on the previous page until whatever long running process Count called completed ?

We think the level of control provided now is optimal. Essentially, you have two options:

  • Move the slow content down below a Suspense boundary.
  • Or, wrap the router navigation call into an extra wrapping useTransition yourself, which gives you back isPending. This lets you “keep track” of the progress on the previous page — for example by showing an inline spinner next to a button.

We might provide a more “global” version of isPending in the future that lets you show a global indicator a la nprogress or hook up to the browser spinning wheel indicator. That’s TBD.

It would be great if startTransition maybe had something like a timeout that can be configured. Perhaps that could incentivize adoption as in the very least if a developer makes a mistake or doesn't anticipate something then there is a safety net ?

We do not think a timeout is the right solution. We’ve actually started with that, and removed it. In practice, it’s very hard to pick the value (5 seconds? 10 seconds? 30 seconds?) if it’s developer’s choice. It also feels arbitrary to the user because it causes existing content to disappear (and be replaced with a spinner) for no discernible reason.

@gne
Copy link

gne commented Apr 20, 2023

@ryanflorence, is it unlikely that your stance on this issue will change?

That makes sense. I've wanted the ability to hold back the router to complete an animation in case the route transition is faster than the animation, or simply avoid flickering a spinner for 10ms.

I think this is a significant limitation in Remix (and React in general, before Suspense / Transitions). Being able to hold back the rendering of state updates is essential to provide smooth transitions. Both when you add animations to mask fetch delays and the fetch is faster than the animation (the issue I believe you describe?), but also more generally to enable "exit animations" where you keep the old state until an animation is done, before rendering the updated state (sliding content out-of/into view, open/close modals etc.).

I agree on your last comment about avoiding render-initiated asynchrony, but I don't think that's necessarily related?

@dani-mp
Copy link

dani-mp commented Apr 24, 2023

How are you folks working around this limitation?

Our use case is: we have a SPA using React Router, a global suspense boundary, and a page showing some content using a suspense-compatible data fetching library. The request is driven by a search param. Changing the search param triggers a new request, and we were expecting that wrapping the setSearchParams call in startTransition would keep the previous data till the new one was ready, avoiding the suspense boundary.

@brophdawg11
Copy link
Contributor

startTransition support is added in remix-run/react-router#10438 and will be available once React Router 6.12.0 and Remix 1.17.0 are released

@brophdawg11 brophdawg11 removed the awaiting release This issue has been fixed and will be released soon label Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feat:routing
Projects
None yet
Development

No branches or pull requests

9 participants