-
-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
useNavigate hook causes waste rendering #7634
Comments
|
I create this test case in v5 and use useHistory hook it doesn't have this behavior, why useNavigate isn't like that? |
Please reopen. This causes half of my application to rerender, just because some searchParams change.
But the current location is only relevant for |
(For future readers: Workarounds shown below) Please consider changing this to the expected behavior. As @benneq said, this causes a lot of rerenders. Why this is a problemExample of a real life use case In my case, I have a list of very render-intense charts. I want to click the charts to navigate to a new page for a detailed view. // child
function DayChart({ dayData: DayData }) {
const navigate = useNavigate();
return (
<div onClick={() => navigate(`/my-route/${dayData.id}`)}>
<Chart data={dayData} />
</div>
);
}
// parent
function DaysList() {
const { days } = useAPI(...)
return <div>{days.map((day) => <DayChart dayData={day} />)}</div>;
} (I'm using memoization in my code, just showing the structure here.) Now, Proposed solutionIf WorkaroundsThere are two workarounds I came up with:
// child
export const DayChart = memo(function DayChart ({
dayData,
navigateToDay,
}: {
dayData: DayData;
navigateToDay: (dayId: string) => void;
}) {
return (
<div onClick={() => navigateToDay(dayData.dayId)}>
<Chart data={dayData} />
</div>
);
});
// parent
function DaysList() {
const { days } = useAPI(...)
const navigate = useNavigate();
const navigateToDay = useCallback(
(dayId: string): void => {
navigate(`/my-route/${dayId}`);
},
[navigate]
);
return <div>{days.map((day) => <DayChart dayData={day} navigateToDay={navigateToDay} />)}</div>;
} BTW: I'm not really sure why this works. You can see that
// child
function DayChart({ dayData: DayData }) {
const [shouldNavigate, setShouldNavigate] = useState<boolean>(false)
return (
<div onClick={() => setShouldNavigate(true)}>
{shouldNavigate && <Navigate to={`/my-route/${dayData.dayId}`} />}
<Chart data={dayData} />
</div>
);
}
// parent
function DaysList() {
const { days } = useAPI(...)
return <div>{days.map((day) => <DayChart dayData={day} />)}</div>;
} |
Could this reopen? |
same problem here, when using |
Re-opening for consideration. I'm not sure that we'll change this behavior but I don't want to close the door on some sort of solution just yet. |
I think if this PR get merged facebook/react#20646 into react it can be used to prevent a lot of rerenders. |
I have a suggestion for a change to useNavigate that might resolve this. For the sake of argument, here is a version of useNavigate stripped down to only the parts relevant to this issue:
I believe the root cause of the re-rendering issue under discussion is that the dependencies array passed to useCallback must include locationPathname for this code to work correctly. I think you could work around this requirement by introducing useRef:
This way, the navigate function will always get an up-to-date value for locationPathname, but the callback won't get regenerated when the location changes. |
Any news on this? The only simple alternative I can think of is to reload the whole page when navigating to avoid two renders but there's no support for |
@timdorr @chaance It seems to me that from the end-user perspective, So, if you don't change result of A separate question is how to implement this semantic at technical level. If you agree with the general idea, I can try to propose a PR (for example, something based on @RandScullard 's idea above). |
When upgrading from V5 to V6, the sub components jump, resulting in re rendering of the whole page, resulting in low performance. Is there a solution If not, you can only roll back the V5 version |
The key to fixing this particular issue is: don't call useContext at all (i.e., don't call export function useNavigate(): NavigateFunction {
let { basename, navigator } = React.useContext(NavigationContext);
let { matches } = React.useContext(RouteContext);
// let { pathname: locationPathname } = useLocation(); // <-- DO NOT DO THIS!!!!!
let routePathnamesJson = JSON.stringify(
matches.map(match => match.pathnameBase)
);
let activeRef = React.useRef(false);
React.useEffect(() => {
activeRef.current = true;
});
let navigate: NavigateFunction = React.useCallback(
(to: To | number, options: NavigateOptions = {}) => {
warning(
activeRef.current,
`You should call navigate() in a React.useEffect(), not when ` +
`your component is first rendered.`
);
if (!activeRef.current) return;
if (typeof to === "number") {
navigator.go(to);
return;
}
// Look up the current pathname *at call-time* rather than current behavior of:
// 1. re-rendering on every location change (incl. query, hash, etc.) (BAD!)
// 2. creating a new navigate function on every pathname change (BAD!)
let { pathname: locationPathname } = (navigator as any).location; // <--- THIS IS THE KEY!!!
let path = resolveTo(
to,
JSON.parse(routePathnamesJson),
locationPathname
);
if (basename !== "/") {
path.pathname = joinPaths([basename, path.pathname]);
}
(!!options.replace ? navigator.replace : navigator.push)(
path,
options.state
);
},
[basename, navigator, routePathnamesJson /*, locationPathname */] // <-- NO NEED FOR THAT!!!
);
return navigate;
} |
@chaance hopefully my above comment opens "the door" to a solution? |
@HansBrende You're right, my useRef suggestion wouldn't prevent the extra renders because of the underlying reference to the LocationContext. Thank you for clarifying! (I think my useRef would help with issue #8349 though.) I have one question about your suggested implementation. You say "The key to fixing this particular issue is: don't call useContext at all" but the first two lines of your function are calls to useContext. Is the key difference that these two calls are referencing context that never (or rarely) changes? |
Yes, exactly! The navigation context, for example, is set once at the top of your application by the
<digression> function usePathname(locationOverride?: Partial<Path>) {
const {navigator: nav} = React.useContext(UNSAFE_NavigationContext) as unknown as {navigator: History};
const [navigator, pathname] = locationOverride ? [null, locationOverride.pathname] : [nav, nav.location.pathname];
const [, triggerRerenderOnlyOnPathnameChange] = React.useReducer(pathnameReducer, pathname || '/');
React.useLayoutEffect(() => navigator?.listen(triggerRerenderOnlyOnPathnameChange), [navigator]);
return pathname || '/';
}
const pathnameReducer = (_: string, upd: Update): string => upd.location.pathname; Then in // let locationFromContext = useLocation(); // <-- Don't do this!
// let location;
// if (locationArg) {
// location = typeof locationArg === "string" ? parsePath(locationArg) : locationArg;
// } else {
// location = locationFromContext;
// }
// let pathname = location.pathname || "/";
// INSTEAD:
let pathname = usePathname(typeof locationArg === "string" ? parsePath(locationArg) : locationArg) </digression> EDIT: I just realized that my "digression" wasn't a digression at all, since |
Ok, so, based on my "EDIT" to my above comment (and thanks to @RandScullard for getting my gears turning), to really fix this problem you will need to remove both the export function useNavigate(): NavigateFunction {
let { basename, navigator } = React.useContext(NavigationContext) ?? {};
invariant(
navigator != null, // useInRouterContext(), <-- useInRouterContext() is just as bad as useLocation()!
`useNavigate() may be used only in the context of a <Router> component.`
);
// let { matches } = React.useContext(RouteContext); // <-- DO NOT DO THIS!!!!!
// let { pathname: locationPathname } = useLocation(); // <-- DO NOT DO THIS!!!!!
// Instead, we need to use Contexts that do not get updated very often, such as the following:
const routeContextRef = React.useContext(NonRenderingRouteContext);
...
return React.useCallback(
(to: To | number, options: NavigateOptions = {}) => {
...
// Look up the current pathname AND ROUTE CONTEXT *at call-time* rather than current behavior of:
// 1. re-rendering on every location change (incl. query, hash, etc.) (BAD!)
// 2. creating a new navigate function on every pathname change (BAD!)
let path = resolveTo(
to,
routeContextRef.current.matches.map(match => match.pathnameBase),
stripBasename((navigator as History).location.pathname || "/", basename)
);
...
navigator.doSomethingWith(path); // ta-da!
},
[basename, navigator, routeContextRef]
);
}
// Where the "NonRenderingRouteContext" is provided, for example, as follows:
const NonRenderingRouteContext = React.createContext<{readonly current: RouteContextObject}>({
current: {
outlet: null,
matches: []
}
});
function RouteContextProvider({value}: {value: RouteContextObject}) {
const ref = React.useRef(value);
React.useLayoutEffect(() => void (ref.current = value)); // Mutating a Ref does not trigger a re-render! 👍
const match = value.matches[value.matches.length - 1];
return ( // Refs are stable over the lifetime of the component, so that's great for Contexts 👍
<NonRenderingRouteContext.Provider value={ref}>
<RouteContext.Provider value={value}>
{match.route.element !== undefined ? match.route.element : <Outlet />}
</RouteContext.Provider>
</NonRenderingRouteContext.Provider>
);
}
function _renderMatches(matches: RouteMatch[] | null, parentMatches: RouteMatch[] = []): React.ReactElement | null {
if (matches == null) return null;
return matches.reduceRight((outlet, match, index) => {
return (
<RouteContextProvider // Simply swap out RouteContext.Provider for RouteContextProvider here!
value={{
outlet,
matches: parentMatches.concat(matches.slice(0, index + 1))
}}
/>
);
}, null);
} [Edited to ALSO remove |
Is anybody measuring performance issues here or just console logging "rerender!" and stressing out? I'd be interested to see an example where this actually affects the user experience. The fact is that |
Just reviewed some of the code samples here. Refs shouldn't be assigned during render, only in effects or event handlers. Maybe the React team has changed their position on this recently, but mutating refs while rendering can cause "tearing" in concurrent mode/suspense. We're just following the rules here: pass dependencies to effects. Since navigate isn't "rendered", it is unfortunate that its dependency on If you've got actual performance problems (not just console logging "rerender!") the simplest solution I can think of right now is to call let AbsoluteNavigate = React.createContext();
function Root() {
let [navigate] = useState(useNavigate())
return (
<AbsoluteNavigate.Provider value={navigate}>
<Routes>
{/*...*/}
</Routes>
<AbsoluteNavigate.Provider>
)
) Anyway, this isn't high priority, but the ref tricks could work if they were only assigned in a useEffect, or there's probably some mutable state on navigator/history that we can use to access the location pathname instead of I personally don't worry about "wasted renders", I trust React is good at what it does and just follow the rules of hooks. I'd likely merge a PR that optimizes this though, so knock yourself out :) |
@ryanflorence IMHO, the question "is anyone measuring performance issues here" completely sidesteps this issue. Any React library worth its salt (that a lot of people are using) should be very conscious of not contributing to "death by a thousand cuts"... that means unnecessary rendering because "who cares, it's not a big performance problem in and of itself" is a big no-no. The very reason the React team is experimenting with context selectors (and had the option for
I didn't. Re: PR: I might get around to one at some point. I do love the concept of react-router, but the heavy usage of frequently changing Contexts at such a fundamental level is a sandy foundation to build upon, and thus a complete non-starter for me, so I've already rewritten the library for my own purposes to cut out most of the major inefficiencies. Now I'm just relying on the |
@ryanflorence My issue: Whenever the component mounts, I make a HTTP request. Because of the double render issue, the component is making two HTTP requests, and for my particular case, we are logging every HTTP request made to our service, but since the component is making an extra request, we are logging two events when there should've been just one. That's why the double render in my use case is a big problem. Surely we can implement some sort of caching to prevent additional requests, but I'm just trying to give one real example. |
@ryanflorence This isn't just an optimization problem. Another problem that caused user-facing bugs while upgrading from v5 to v6, was that a Here's the scenario: We have a field with an onComplete handler that, when triggered, navigates to a new route.
A work-around was to decouple the
|
@ryanflorence I agree with @deleteme, I am really more concerned with effects being triggered. Additional renders don't create a problem in my use case. I still think the effect problem could be solved as in #7634 (comment). But good point about setting the ref in an effect, not inline in the render. (Full disclosure: I have not tested this.) |
Just chiming in, came to this issue after running into a race condition. I was optimistically updating state and navigating away from the page to an updated page slug, but because |
What do you think about a hook like useAbsoluteNavigate() or so. The hook would be just like useNavigate() but would not support relative paths. Then you wouldn't need useLocation in the hook and would save some rerenderer. |
Or can we just use window.location.pathname instead of useLocation().pathname? |
Thanks @brophdawg11 I'll take a look. |
Aligning with Remix here - will close this issue with |
🤖 Hello there, We just published version Thanks! |
🤖 Hello there, We just published version Thanks! |
@brophdawg11 Gave a quick try using |
@braincore can you please provide a code sandbox example. |
@hichemfantar Here's a sandbox: https://codesandbox.io/s/admiring-dan-uvyqeg?file=/src/App.js The |
@braincore I think there's some confusion with re-renders versus navigate identity stability, so per this comment, what we did here was make the identity of the Most of the actual issues reported were regards to the
But, If you want to optimize further and avoid re-renders, there are two suggested solutions above to either use a context-provided |
@brophdawg11 Thanks for the clarification. I've given this another try, and the stable navigate is sufficient for my use case. I'm able to reduce renders using typical react memoization strategies. Thanks! |
Is there any way I could wrap <RouterProvider router={router}>
<RefreshToken />
<Toaster />
</RouterProvider> Typescript error: Purpose:I want to run some function on interval and which also uses Issue:
|
Put those components in a root layout route around your whole app:
|
I'm late to the party, but I'm still not completely satisfied with the current fix. Can someone from the team (@brophdawg11 @ryanflorence) explain why the |
@cbreezier Thats because the current fix doesn't address the original issue; that navigation updates force re-rendering. The fix only made the identity of the
This is how the maintainers (@ryanflorence) feel about this issue. You should embrace rerenders because "they are at the heart of react and are usually not an issue unless you have expensive/slow components" (#10756 @brophdawg11). @HansBrende commented the full fix a few comments later #7634 (comment) (I haven't tested yet). I don't know why this can't be addressed, but it does feel like stubbornness. There are numerous questions in issues and discussions about this. Context change re-renders every consumer. His follow up response to the maintainers is poignant:
|
This comment was marked as off-topic.
This comment was marked as off-topic.
What I noticed on
To:
And all of the sudden my Navbar was blinking like mounting and remounting, I am still debugging, checking if this is only because of Strict mode in development or something. I suspect it's related to this issue because I use |
After reading all of this, I'm still unsure about what would be the best practice to solve the following: I have a paginated list view. The current page is indicated by a URL search parameter ( |
I still think there should be a separate It would only accept absolute paths to remove the relative path dependency. If others agree, I might consider making an MR when I have some free time. |
…ders (we always set absolute URLs). See this for more information: remix-run/react-router#7634 (comment)
Here's my working solution using
I'm on v6.23.1, btw. |
Instead of passing the `closeModal` function, let's just pass the `navigateToOnClose` parameter. The redirection logic will be handled in one place. The`react-router-dom` package has been updated to version `6.26.0` because there was previously a problem with the useNavigate hooks. `useNavigate` caused the component to be re-rendered. This is probably related to this issue - remix-run/react-router#7634
I'm am working with |
We've ended up wrapping useLocation and useNavigate into useDeferredValue to improve INP. |
Version
6.0.0-beta.0
Test Case
I create a pure component, there is no reason to execute again when i want to navigate to other page using useNavigate , I think useNavigate hook cause waste rendering on pure components, when using useHistory hook of last version(5) it doesn't have this behavior
I have a question here for more detail
Am i wrong or something changed?
https://codesandbox.io/s/dawn-wave-cy31r?file=/src/App.js
Steps to reproduce
Create pure component
Use useNavigate hook inside it
Expected Behavior
Navigate to other page should not cause execute of pure components when props are not changed
Actual Behavior
Navigate to other page in pure component causes waste rendering, component execute again even when props are same
The text was updated successfully, but these errors were encountered: