`clientLoader` revalidate even though `shouldRevalidate` returns `false`

[prxg22]

I'm using React Router as a...

framework

Reproduction

1. Go to this stackblitz
2. On the reproduction browser, navigate to /main/view
3. Click the toggle theme button
4. Check the child clientLoader count increase

System Info

System:
    OS: macOS 14.6.1
    CPU: (8) arm64 Apple M1
    Memory: 129.34 MB / 8.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.18.1 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 10.8.2 - /usr/local/bin/npm
    pnpm: 8.15.7 - /usr/local/bin/pnpm
  Browsers:
    Chrome: 131.0.6778.205
    Safari: 17.6
  npmPackages:
    @react-router/dev: ^7.1.0 => 7.1.0 
    @react-router/node: ^7.1.0 => 7.1.0 
    @react-router/serve: ^7.1.0 => 7.1.0 
    react-router: ^7.1.0 => 7.1.0 
    vite: ^5.4.11 => 5.4.11

Used Package Manager

npm

Expected Behavior

As reported in remix-run/remix#10234, single fetch feature introduced an unexpected behavior on clientLoader revalidation when using ssr = false strategy.

When a child route's shouldRevalidate function returns false, I expected that this route's clientLoader would not be triggered after a fetcher's submission in a parent route.

In my example / route has a fetcher.Form, that when submitted, should run its clientAction and revalidate that route's clientLoader, but routes /main and /main/view should not be revalidate, since their shouldRevalidate function will return false for that form submission.

Actual Behavior

After a fetcher submission in a parent route, child clientLoaders are being revalidated, even though their route's shouldRevalidate returns false.

In my example, you can check /main and /main/view counters increasing after each action dispatched by the parent route's fetcher.

[aland-x]

Reported this also in #12551 unfortunately I think @timdorr misunderstood the issue. I hope this makes it more clear.

[lukejagodzinski]

I have this issue as well. I think it was fine in RR 7.0 but got broken in 7.1. However, my use case was a little bit different. In RR 7.0, when action failed with error 422 or whatever other 4XX, it didn't revalidate. Now it revalidates. So I thought I will fix it with the shouldRevalidate and then I discover this bug and I see I'm not the only one. The question also is, should it revalidate on error? I think, I heard in one of the streams that it will not revalidate on action error.

[matheuscscp]

@timdorr Can you please look into this? 🙏 😸

[lukejagodzinski]

I've just checked code and in deed it shouldn't revalidate on 4XX or 5XX. You can see it here:

react-router/packages/react-router/lib/router/router.ts

Lines 4219 to 4222 in 79e6bc9

	let actionStatus = pendingActionResult
	? pendingActionResult[1].statusCode
	: undefined;
	let shouldSkipRevalidation = actionStatus && actionStatus >= 400;

[prxg22]

@lukejagodzinski according to this doc, it should not revalidate if you throw or return a 4xx/5xx Response.

[miniplus]

This is even happening without submissions.

// route: /parent
export function clientLoader() {
    console.log('parent clientLoader ran')
}

export default function Parent() {
    return (
        <>
            <Link to='./child'>Child</Link>
            <Outlet />
        </>
    )
}

// route: /parent/child
export default function Child() {
    return (
        <>
            <Link to='..'>Back</Link>
            <p>Child content</p>
        </>
    )
}

Whenever navigating to the child route, the parent clientLoader is running.
When navigating from the child back to the parent it is not.

Adding shouldRevalidate to the Parent route as follows has no effect and still allows the Parent clientLoader to run.

export const shouldRevalidate = ({
  nextUrl,
  defaultShouldRevalidate,
}) => {
  if (nextUrl.pathname === "/parent/child") {
    console.log("shouldRevalidate: false");
    return false;
  }

  return defaultShouldRevalidate;
};

[TimurTimergalin]

I encountered this issue as well. It seems like shouldRevalidate has no effect on clientLoader whatsoever. An example as simple as this:

// routes.ts

export default [
    layout("layouts/layout.tsx", [
        route("/route1", "routes/route1.tsx"),
        route("/route2", "routes/route2.tsx")
    ])
] satisfies RouteConfig;

// layouts/layout.tsx

export async function clientLoader() {
    console.log("Loader called")
}

export function shouldRevalidate() {
    console.log("shouldRevalidate called")
    return false
}

export default () => <Outlet/>

// routes/route1.tsx

export default () => (
    <>
        <p>Route 1</p>
        <Link to={"/route2"}>To route 2</Link>
    </>
)

// routes/route2.tsx

export default () => (
    <>
        <p>Route 2</p>
        <Link to={"/route1"}>To route 1</Link>
    </>
)

has this problem: when navigating between /route1 and /route2, both shouldRevalidate and clientLoader of parent layout are called, even though shouldRevalidate returns false. Adding shouldRevalidate to children modules does not work either.

[lukejagodzinski]

Can we get at least some acknowledgment that someone is working on it? It's pretty serious issue. It's not just app doing more work unnecessarily. It's also causing the action return data to be abandoned because of revalidation. If I don't store it in some state then it's gone and I have no info about the action error or return value. It basically makes app unusable. I'm really surprised that this error doesn't get more attention.

Can we somehow help? It's not like I just only want to complain but maybe we could actively solve this issue together? If someone from the Remix team points us to the source of issue then we might be able to contribute.

[MadsBuchmann]

Can we get at least some acknowledgment that someone is working on it? It's pretty serious issue. It's not just app doing more work unnecessarily. It's also causing the action return data to be abandoned because of revalidation. If I don't store it in some state then it's gone and I have no info about the action error or return value. It basically makes app unusable. I'm really surprised that this error doesn't get more attention.

Can we somehow help? It's not like I just only want to complain but maybe we could actively solve this issue together? If someone from the Remix team points us to the source of issue then we might be able to contribute.

Just want to chime in on this one. I would love helping out if possible! It's a serious issue in our application right now giving us +5 seconds load times on every page change - leading to some hacky solutions at the moment to circumvent the issue. Also, if there's any ideas for temporary workarounds i will happily test them out in our setup 😄

I also tried to downgrade to v7.0.0 and the issue also seems to persist here. For context we have ssr = false.

[TimurTimergalin]

@MadsBuchmann one workaround would be to prevent revalidation manually with sessionStorage:

const module = "A string, that is unique for each module"
const loaderDataPostfix = "loaderData"
const shouldRevalidatePostfix = "shouldRevalidate"

export function shouldRevalidate(args: ShouldRevalidateFunctionArgs) {
    const res = /* Your usual code. Make sure it is a boolean */
    sessionStorage.setItem(module + shouldRevalidatePostfix, String(res))  // Remove when the bug is fixed
    return res
}

export async function clientLoader(args: Route.ClientLoaderArgs) {
    if (sessionStorage.getItem(module + shouldRevalidatePostfix) !== "true") {  // Prevent revalidation, Remove when the bug is fixed
        return JSON.parse(sessionStorage.getItem(module + loaderDataPostfix)!)
    }
    const res = /* Your usual code - make sure it can be used with JSON.stringify*/
    sessionStorage.setItem(module + loaderDataPostfix, JSON.stringify(res))  // Remove when the bug is fixed
    return res
}   

This is somewhat hacky (and ultimately something react-router is created to replace), so I wish this bug is fixed soon. Hope the workaround will be useful though - it worked for me.

[MadsBuchmann]

@TimurTimergalin thanks! That's pretty clever. We're doing some very simple in-memory caching at the moment. Because we're always returning the same object, we're avoiding unnecessary re-renders in the application.

let cachedClientLoader: undefined | T; 

export function clientLoader(...): T {
    if (cachedClientLoader) return cachedClientLoader; 

    cachedClientLoader = /* usual code resulting in an object to be returned with type T*/
    return cachedClientLoader
}

export function shouldRevalidate(...) {
   const revalidate: bool = /* bool defining whether you want to revalidate*/
   if (revalidate) {
      cachedClientLoader = undefined; 
   }
}

[matheuscscp]

nice workaround, chaps!

but I'd really appreciate a proper fix here :)

I'm more than willing to contribute this fix myself, but I'm like a blinded person in a shooting spree :( if anybody has any pointers I'd be more than happy to open a PR and work on it 🙏

as a maintainer of another largely used open source project I fully appreciate that there's many things to look at and all help is always welcome 🌹

[lukejagodzinski]

OK, I actually have to take back what I said. My scenario is a little bit different from what you guys experience and it's not critical for my app working. Today, I was creating reproduction repository for this issue and noticed that it's similar but not exactly the same issue. There is still problem with the unnecessary revalidation happening but I can easily fix it with shouldRevalidate export where you can't. If anyone is curious I filed a bug report: #13062

[MadsBuchmann]

I think i may have discovered an additional detail regarding this issue or perhaps something in my understanding of how revalidation works is wrong.

I have two routes:

• $userName.($country): here shouldRevalidate always returns false
• $userName.($country).$municipality: here shouldRevalidate always returns true

If i go from /mads/denmark to /mads/sweden then it never revalidates i.e. works as i would expect! Nice!

However if i change between municipalities then the route $userName.($country) starts revalidating again for example by changing from /mads/denmark/aarhus to mads/denmark/copenhagen. Not working as expected, as i would like the route $userName.($country) to never revalidate and $userName.($country).$municipality to revalidate.

But perhaps my understanding is off, if so, please correct me.