-
-
Notifications
You must be signed in to change notification settings - Fork 10.5k
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
clientLoader
revalidate even though shouldRevalidate
returns false
#12607
Comments
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 |
@timdorr Can you please look into this? 🙏 😸 |
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
|
@lukejagodzinski according to this doc, it should not revalidate if you throw or return a 4xx/5xx Response. |
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. Adding export const shouldRevalidate = ({
nextUrl,
defaultShouldRevalidate,
}) => {
if (nextUrl.pathname === "/parent/child") {
console.log("shouldRevalidate: false");
return false;
}
return defaultShouldRevalidate;
}; |
I encountered this issue as well. It seems like // 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 |
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 |
@MadsBuchmann one workaround would be to prevent revalidation manually with 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. |
@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;
}
} |
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 🌹 |
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 |
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:
If i go from However if i change between municipalities then the route But perhaps my understanding is off, if so, please correct me. |
I quickly wrote a Vite Plugin that does this solution, to avoid modifying all the files (then having to revert the changes once the bug is fixed). It does the transform "on-the-fly". The code is not very well written but it seems to work for the most part. import type {Plugin} from "vite";
import {parse, print, types} from "recast";
import {visit} from "ast-types";
import babelTsParser from "recast/parsers/babel-ts";
const b = types.builders;
export function VitePluginRevalidate(): Plugin {
return {
name: "vite-plugin-revalidate",
enforce: "pre",
transform(code, id) {
if (!id.endsWith(".tsx") || id.includes('node_modules')) return null;
let ast = parse(code, {
parser: babelTsParser
});
let hasShouldRevalidate = false;
let hasClientLoader = false;
visit(ast, {
visitFunctionDeclaration(path) {
if (path.node.id?.name === "shouldRevalidate") {
hasShouldRevalidate = true;
const body = path.node.body.body;
const generatedFunction = b.functionDeclaration(
b.identifier("__generated"),
[],
b.blockStatement(body)
);
path.node.body.body = [
generatedFunction,
b.variableDeclaration("const", [
b.variableDeclarator(b.identifier("revalidate"), b.callExpression(b.identifier("__generated"), []))
]),
b.ifStatement(
b.identifier("revalidate"),
b.blockStatement([
b.expressionStatement(
b.assignmentExpression("=", b.identifier("cachedClientLoader"), b.identifier("undefined"))
)
])
)
];
} else if (path.node.id?.name === "clientLoader") {
hasClientLoader = true;
const body = path.node.body;
const isAsync = path.node.async;
const generatedFunction = b.functionDeclaration(
b.identifier("__generated"),
[],
b.blockStatement(body.body)
);
if (isAsync) {
generatedFunction.async = true;
}
const callExpr = b.callExpression(b.identifier("__generated"), []);
body.body = [
b.ifStatement(
b.binaryExpression(
"!==",
b.identifier("cachedClientLoader"),
b.identifier("undefined")
),
b.blockStatement([
b.returnStatement(b.identifier("cachedClientLoader"))
])
),
generatedFunction,
b.variableDeclaration("const", [
b.variableDeclarator(b.identifier("result"), isAsync ? b.awaitExpression(callExpr) : callExpr)
]),
b.expressionStatement(
b.assignmentExpression("=", b.identifier("cachedClientLoader"), b.identifier("result")),
),
b.returnStatement(b.identifier("cachedClientLoader"))
]
}
return false;
}
});
if (!hasShouldRevalidate || !hasClientLoader) return null;
ast.program.body.unshift(
b.variableDeclaration("let", [b.variableDeclarator(b.identifier("cachedClientLoader"))])
);
return {
code: print(ast).code,
map: null,
};
}
};
} Save it in the project folder as export default defineConfig({
plugins: [
VitePluginRevalidate(),
reactRouter(),
tsconfigPaths()
],
... |
I'm struggling with this issue as well, the only way I've been able to resolve it is by defining // Routes
export default [
index('./routes/index.tsx'),
route('entity', './routes/entity/entityLayout.tsx', [
index('./routes/entity/entityIndex.tsx'),
route(':entityId', './routes/entity/entity.tsx'),
]),
route('*', './routes/notFound.tsx'),
] satisfies RouteConfig; I have to export
My expectation (hope?) was that I would only need to apply this on |
Hey folks - for SPA mode ( For Update: Remix backport PR: remix-run/remix#10527 |
I'm using React Router as a...
framework
Reproduction
/main/view
toggle theme
buttonclientLoader
count increaseSystem Info
Used Package Manager
npm
Expected Behavior
As reported in remix-run/remix#10234, single fetch feature introduced an unexpected behavior on
clientLoader
revalidation when usingssr = false
strategy.When a child route's
shouldRevalidate
function returnsfalse
, I expected that this route'sclientLoader
would not be triggered after afetcher
's submission in a parent route.In my example
/
route has afetcher.Form
, that when submitted, should run itsclientAction
and revalidate that route'sclientLoader
, but routes/main
and/main/view
should not be revalidate, since theirshouldRevalidate
function will returnfalse
for that form submission.Actual Behavior
After a
fetcher
submission in a parent route, childclientLoaders
are being revalidated, even though their route'sshouldRevalidate
returns false.In my example, you can check
/main
and/main/view
counters increasing after each action dispatched by the parent route'sfetcher
.The text was updated successfully, but these errors were encountered: