Render loop when revalidating a page that has Suspense/Await Elements on it

[lauhon]

When using the useRevalidator hook on a page that renders an async value with Suspense/Await. The component will render infinitely.

The test in the PR will actually go through, as I did not know how to accurately assert this. When you check the browser tab that gets opened through app.poke you will see a huge number of renders and the counter rising in the logs.

[remix-cla-bot[bot]]

Hi @lauhon,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at [email protected].

Thanks!

- The Remix team

[changeset-bot[bot]]

⚠️ No Changeset found

Latest commit: 437a66f4db69cfa63e02bf20e2bc730c156f914c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[remix-cla-bot[bot]]

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

[brophdawg11]

Can you describe your real world use case here?

In this simplified example, this is a combination of the effect changing state and the fact that you are creating a net-new Promise on every render and handing it to Await. When Await receives a net-new promise, it cannot unwrap it synchronously so it needs to throw it for at least one render cycle until it can "read" the resolved value. Normally, Suspense would then not re-render until there was a resolved value.

But you're setting state, causing a re-render, and then giving it a net-new promise so it has to throw again to track it.

The loop doesn't really have to do with revalidate and still happens in this simplified setup:

export default function Spaghetti() {
  const renderCounter = useRef(0);
  const [, setCount] = useState(0);
  renderCounter.current = renderCounter.current + 1;

  console.log("rendering, renderCounter.current =", renderCounter.current);
  useEffect(() => {
    setCount(1);
  }, []);

  return (
    <>
      <p>[{renderCounter.current}]</p>
      <Suspense>
        <Await resolve={Promise.resolve(1)}>
          {(val) => (
            <div>
              <p>Async val: {val}</p>
            </div>
          )}
        </Await>
      </Suspense>
    </>
  );
}

Lifting the promise so it's created outside of render fixes it:

let promise = Promise.resolve(1);
// Then use <Await resolve={promise}>

The way to "lift" this promise in Remix is to use a loader + defer which also resolves the looping issue:

import { defer } from "@remix-run/node";
import { useRevalidator, Await, useLoaderData } from "@remix-run/react";
import { useEffect, useRef, useState, Suspense } from "react";

export function loader() {
  return defer({
    promise: Promise.resolve(Math.random()),
  });
}

export default function Spaghetti() {
  let data = useLoaderData();
  let { revalidate } = useRevalidator();
  const renderCounter = useRef(0);
  const [, setCount] = useState(0);
  renderCounter.current = renderCounter.current + 1;

  console.log("rendering, renderCounter.current =", renderCounter.current);
  useEffect(() => {
    revalidate();
    setCount(1);
  }, []);

  return (
    <>
      <p>[{renderCounter.current}]</p>
      <Suspense>
        <Await resolve={data.promise}>
          {(val) => (
            <div>
              <p>Async val: {val}</p>
            </div>
          )}
        </Await>
      </Suspense>
    </>
  );
}

[lauhon]

Thanks for reacting so quickly to this!

My use case is that I am getting a promise via defer & modifying it in the frontend. Like you describe, when I lift the promise creation in my example into a loader the problem is fixed.

But it re-appears as soon as I edit the promise via .then().

I guess it's solvable easily by performing the modification as well in the loader, but I still find it interesting that this causes the render loop.

Or is what I am describing simply an anti pattern?

Anyways I will update the test so it includes what I described in the comment: (when you comment out revalidate() the endless render stops)

import { Await, defer, useLoaderData, useRevalidator } from "@remix-run/react";
import { Suspense, useEffect, useRef, useState } from "react";

export const loader = () => {
  const meatball = Promise.resolve(1);

  return defer({ meatball });
};

export default function Spaghetti() {
  const { meatball } = useLoaderData<typeof loader>();

  const { revalidate } = useRevalidator();
  const renderCounter = useRef(0);
  const [_, setCount] = useState(0);

  renderCounter.current++;

  useEffect(() => {
    revalidate();
  }, []);

  console.log("render", renderCounter.current);

  return (
    <>
      <p data-testid="render-count">{renderCounter.current}</p>
      <Suspense>
        <Await resolve={meatball.then((amount) => amount * 2)}>
          {(val) => (
            <div>
              <p>Async val: {val}</p>
            </div>
          )}
        </Await>
      </Suspense>
    </>
  );
}

[lauhon]

The loop doesn't really have to do with revalidate and still happens in this simplified setup:

The setState was an attempt of mine to force the render count to be reflected in the HTML to be able to assert something. The render loop also occurs without useState and stops happening as soon as I remove revalidate()

I will remove everything un-essential from my test

[brophdawg11]

I am getting a promise via defer & modifying it in the frontend

This sounds like the root of the issue to me - Await (and eventually React.use I think) require stable promises so they can know if they've already seen them or not, so creating a net new promise on each render is always going to be problematic.

That said, I would have expected a simple useMemo to fix this but I'm still seeing the loop with that introduced - even when the promise in the useMemo dependency array doesn't change which is odd. I'm able to work around the issue by adding a field to the memoized promise so I can know if I've seen it before and skip the re-creation, but it feels like I shouldn't have to - that's sort of the purpose of useMemo:

  let memoPromise = useMemo(() => {
    if (meatball.seen) {
      console.log("Already seen");
      // This allows us to short circuit the loop
      return meatball;
    }
    meatball.seen = true;
    return meatball.then((amount) => amount * 2);
  }, [meatball]);

I also thought it might have something to do with revalidating in the first render but even if I put that behind a button click it triggers the loop when clicked. So I would say that revalidating on first render might be an anti-pattern, but this issue still happens in the very normal pattern of revalidating later on.

So this will require some more digging to get to the root of the issue - it feels like useMemo is not behaving correctly here, but it willr take a simplified example to see if that's the case of it maybe we're doing something weird in Await that's messing up the useMemo dependency check...

[lauhon]

Super interesting.

I'm trying my best to help here. Let me know if I can provide a specific scenario so you can just check it out..

I looked a bit and found out that <Await> from react-router uses <ResolveAwait> which in turn uses useAsyncDatawhich is basically just a react context. On the react docs I found the following caveat of the useContext hook:

"The previous and the next values are compared with the Object.is comparison. Skipping re-renders with memo does not prevent the children receiving fresh context values."

I'm sure that there is a reason behind this, but to me, as an outsider, it seems strange that the Await component needs to rely on a react context, as the promise itself is simply a prop. Maybe that will lead to a solution there?

I'm happy to fidget around a bit, let me know what you think! :)