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
Render loop when revalidating a page that has Suspense/Await Elements on it #9046
base: main
Are you sure you want to change the base?
Conversation
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 If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run. Thanks! - The Remix team |
|
1d6eb39
to
79b340e
Compare
Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳 |
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 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 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 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>
</>
);
} |
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 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: 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>
</>
);
}
|
The I will remove everything un-essential from my test |
This sounds like the root of the issue to me - That said, I would have expected a simple 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 |
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 I'm sure that there is a reason behind this, but to me, as an outsider, it seems strange that the I'm happy to fidget around a bit, let me know what you think! :) |
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.