-
-
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
useSearchParams Bug Fix | Do not regenerate the setSearchParams function when the searchParams change #10740
base: dev
Are you sure you want to change the base?
Conversation
…s not regenerated each time the searchParams change.
Hi @rwilliams3088, Welcome, and thank you for contributing to React Router! 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 [email protected]. Thanks! - The Remix team |
|
Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳 |
can this be merged? |
It would be nice to hear something from maintainers, whether this is being considered or not. |
@rwilliams3088 Could you please fix the tests so that your fix becomes ready to merge? It would be super useful. Thank you for your work. |
If you pull it down and run it, you'll find all unit tests are passing. The issue on github is a memory issue due to whatever configuration they have setup for running the unit tests on here. However, since it has been a bit, I've gone ahead and update by branch with the latest. Maybe they've fixed their test config by now. |
packages/react-router-dom/index.tsx
Outdated
let searchParamsRef = React.useRef<URLSearchParams>(defaultSearchParamsRef.current); | ||
|
||
let location = useLocation(); | ||
let searchParams = React.useMemo( | ||
() => | ||
() => { | ||
// Only merge in the defaults if we haven't yet called setSearchParams. | ||
// Once we call that we want those to take precedence, otherwise you can't | ||
// remove a param with setSearchParams({}) if it has an initial value | ||
getSearchParamsForLocation( | ||
searchParamsRef.current = getSearchParamsForLocation( | ||
location.search, | ||
hasSetSearchParamsRef.current ? null : defaultSearchParamsRef.current | ||
), | ||
); | ||
return searchParamsRef.current; | ||
}, | ||
[location.search] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fairly certain that while this works in most cases, it's not transition-safe.
Quoting the rules for useRef
:
Do not write or read ref.current during rendering, except for initialization. This makes your component’s behavior unpredictable.
Writing to searchParamsRef
during render like this violates those rules, and I wouldn't be surprised if it leads to inconsistent states when using startTransition
, which may be a concern for RRv7.
I think the correct solution in the future would be to wrap setSearchParams
in useEffectEvent
, but that isn't stable yet 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about this (it is derived from older code, but the principle is obvious):
const searchParamsRef = React.useRef();
const [searchParams, setRawSearchParams] = React.useState();
React.useEffect(() => {
searchParamsRef.current = getSearchParamsForLocation(
location.search,
defaultSearchParamsRef.current
)
setRawSearchParams(searchParamsRef.current);
},
[location.search]);
let navigate = useNavigate();
let setSearchParams = React.useCallback<SetURLSearchParams>(
(nextInit, navigateOptions) => {
const newSearchParams = createSearchParams(
typeof nextInit === "function" ? nextInit(searchParamsRef.current) : nextInit
);
navigate("?" + newSearchParams, navigateOptions);
},
[navigate]
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fairly certain that while this works in most cases, it's not transition-safe.
Quoting the rules for
useRef
:Do not write or read ref.current during rendering, except for initialization. This makes your component’s behavior unpredictable.
Writing to
searchParamsRef
during render like this violates those rules, and I wouldn't be surprised if it leads to inconsistent states when usingstartTransition
, which may be a concern for RRv7.I think the correct solution in the future would be to wrap
setSearchParams
inuseEffectEvent
, but that isn't stable yet 🙃
In context, if you take a look at the example code in question, they are saying not to set the reference's value during the body of the function component - or equivalently the render function on a class component. It is not saying that you should not assign references within the context of a useEffect (or useMemo specifically in this case). That is a standard usage for a reference: useEffect doesn't run during rendering https://react.dev/reference/react/useEffect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about this
I think that should be alright, would just need to validate that it works
It is not saying that you should not assign references within the context of a useEffect (or useMemo specifically in this case)
You're right in that you can write to refs in useEffect
, but you still shouldn't do it in useMemo
since the callback of a useMemo
does get ran during render (if its dependencies change ofc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right in that you can write to refs in
useEffect
, but you still shouldn't do it inuseMemo
since the callback of auseMemo
does get ran during render (if its dependencies change ofc)
useMemo
executes between re-renders just like useEffect
: https://react.dev/reference/react/useMemo
useMemo is a React Hook that lets you cache the result of a calculation between re-renders.
All hooks behave like this; executing asynchronously from the rendering logic. If hooks were just standard function calls that executed synchronously, then there wouldn't be any reason to dub them hooks
and demand that they follow special rules. Think of useMemo
as nothing more than a specialized case of useEffect
that focuses on caching a value, with cache invalidation based upon the dependency list. Similarly, useCallback
is just a specialized case of useEffect
for caching a function, which can be invalidated based upon the dependency list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useMemo executes between re-renders just like useEffect
All hooks behave like this; executing asynchronously from the rendering logic.
This isn't true. Effects are asynchronous, but memos are synchronous and are evaluated during render (if their dependencies change). This can be proved by looking at the ReactFiberHooks.jsx
implementation of useMemo
. Notice that the execution of nextCreate
isn't deferred, rather it's called inline during render.
Also, the 'between rerenders' part means that the value is cached between rerenders, not that the function is called between rerenders.
If hooks were just standard function calls that executed synchronously, then there wouldn't be any reason to dub them
hooks
and demand they follow special rules.
In the case of useMemo
, the special thing that makes it a hook is the fact that it caches previous executions, as the cache is stored as part of the hook tree's state. The function you pass to a memo should have 0 side effects, so it shouldn't care how it's executed, but if memos themselves were asynchronous then you'd have all sorts of stale closure problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly I don't think that setting refference during useMemo could cause any issues cos it is non-reactive type and it would need to hit same event loop tick in specific order (possibility close to zero), but since this is widely used public repo and it is close to zero, but not a zero, I would choose recommended solution just to avoid these long threads..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Brendonovich I did some more testing, and it would appear that you are correct - useMemo
does execute synchronously with the re-rendering process. My test code:
export interface MemoTestProps {
n: number;
}
export const MemoTest = (props: MemoTestProps) => {
const [i, setIndex] = useState<number>(0);
const renderCount = useRef<number>(0);
const memoCount = useRef<number>(0);
renderCount.current += 1;
console.log(`renderCount before useMemo: ${renderCount.current}, memoCount: ${memoCount.current}`);
const cachedValue = useMemo(() => {
memoCount.current += 1;
console.log(`updated memoCount: ${memoCount.current}`);
return i;
}, [i]);
console.log(`renderCount after useMemo: ${renderCount.current}, memoCount: ${memoCount.current}`);
useEffect(() => {
if (i < props.n) {
setIndex(prev => prev + 1);
}
}, [i, setIndex, props.n]);
return (
<div>
<p><label>Render Count:</label> <span>{renderCount.current}</span></p>
<p><label>Memo Count:</label> <span>{memoCount.current}</span></p>
<p><label>Cached Value:</label> <span>{cachedValue}</span></p>
</div>
);
};
The memoCount
can be observed to always update synchronously with the renderCount
in the log output.
I will find some time to look back over this to see if there is a better solution then; although the current solution is still far better than leaving the broken behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that setting reference during useMemo could cause any issues cos it is non-reactive type and it would need to hit same event loop tick in specific order (possibility close to zero)
Event loop ticks aren't the problem, writing to a ref during render is simply not good as not all results of renders are actually used, so just because a value is calculated during render doesn't mean the user will see that value, as during a Transition the results of that render may get thrown away by React, and yet if you put that value into a ref during render it'll stick around and be accessible in other places, even though that render was never used.
I think the useEffect
solution should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the PR so that it uses a useEffect to updated the reference.
Will this merged soon? |
I just submitted some revisions based upon the feedback above; waiting for the updates to be reviewed. |
Thank you!! :-) |
Who needs to review this? Is it @Brendonovich? |
Please merge this or solve the issue in another way. When keeping searchparams up to date on every navigation, every navigation leads to ALL components rerendering. That is a no go. We are seriously considering migrating to tanstack router. |
@rwilliams3088 any update ? |
@NicoAdrian I only created the PR; it's up to whoever is maintaining this project to review and merge it. I have no clue what's going on behind the scenes. I'd have thought by now there'd have been some kind of response from the react-router team.... |
@brookslybrand Hello, any chance for this PR to be merged ? |
I was encountering re-rendering errors in React and narrowed down the root cause to the useSearchParams() function. It turns out that the present implementation will unnecessarily regenerate the setSearchParams function every time the searchParams reference changes. If you are passing around the setSearchParams function to child components, you will find that the child components keeps getting re-rendered whenever the searchParams change - even if the child component isn't dependent upon the searchParams.
Example: