[Bug]: Make setSearchParams stable?

[kentcdodds]

What version of React Router are you using?

6.7.0

Steps to Reproduce

1. Use setSearchParams in anything that needs a dependency array and add it to avoid issues.
2. Notice that the presence of setSearchParams in the dependency array triggers the useEffect any time the searchParams change (for example) to run again

Expected Behavior

I expect to be able to use setSearchParams in a useEffect and not have that useEffect re-run just because search params changed.

Actual Behavior

This is happening because setSearchParams allows you to pass a function callback which receives the current searchParams so setSearchParams changes whenever the searchParams change. This is a nice API, but leads to unexpected re-runs of things using setSearchParams.

An alternative is to use a ref to keep track of the latest searchParams and pass that ref.current instead so searchParams doesn't need to be listed in the nextInit call. I can't think of a scenario where this would break anything.

The code I'm talking about is here:

https://github.com/remix-run/react-router/blob/0ce0e4c728129efe214521a22fb902fa652bac70/packages/react-router-dom/index.tsx#L872-L881

[timdorr]

Some historical context on this:

9304

9912

8908 / #9851

[max-pcentra]

Also, calling setSearchParams will re-render the whole route. It would be very helpful if at least there would be some sort of flag on each route config, similar to shouldRevalidate, that let's you opt out of such re-renders.

[zhengjitf]

I feel that all functions in hooks, especially those we’ll call in other hooks or as props to a component, are supposed to be stable. we just need to be able to get the latest states when these functions are called.

The following is a simple solution to create stable functions:

/* in a hook */

const fn = { /*... */ }

const fnRef = useRef(fn)
fnRef.current = fn
const stableFn = useCallback((...args) => fnRef.current(...args), [])

// then, we can use stableFn

[github-actions[bot]]

This issue has been automatically marked stale because we haven't received a response from the original author in a while 🙈. This automation helps keep the issue tracker clean from issues that are not actionable. Please reach out if you have more information for us or you think this issue shouldn't be closed! 🙂 If you don't do so within 7 days, this issue will be automatically closed.

[dennisameling]

Please don't close, I literally ran into this issue today 🙏🏼

[jamesa3]

We are still affected by this bug. This issue should not be closed until a fix is published.

[KostiantynO]

I use useCallback and it works!

const updateSearchParams = useCallback(setSearchParams, []); 

Please kindly consider to make updateFunction a stable reference :)

[maastrich]

Also, if multiple updates happen in the same render cycle, using the new value as a dependency of the callback causes it to no be provided within the prev param of the n+1 setSearchParams call

[lucabergamini]

This is what I'm currently using as a workaround (following @zhengjitf stable function suggestion):

import { useCallback, useEffect, useRef } from "react";
import { useSearchParams as useSearchParamsOrig } from "react-router-dom";

export function useSearchParams() {
  const [searchParams, setSearchParams] = useSearchParamsOrig();
  // Store a reference to the setter
  const setSearchParamsRef = useRef(setSearchParams);
  // Update the reference when the setter changes
  useEffect(() => {
    setSearchParamsRef.current = setSearchParams;
  }, [setSearchParams]);
  // Return a stable setter (which has no dep on searchParams)
  const setSearchParamsStable = useCallback(
    (...args: Parameters<typeof setSearchParams>) =>
      setSearchParamsRef.current(...args),
    []
  );
  return { searchParams, setSearchParams: setSearchParamsStable };
}

in case anybody is looking for the same workaround. It works for my use case, but I've not tested corner cases.

[maastrich]

@lucabergamini useEffect is no good, it would be best at the root off the component as the ref would be instantly updated instead of waiting for the render cycle to end in order to run the effects.

[lucabergamini]

Do you mean a setSearchParamsRef.current=setSearchParams in the body of the hook to replace the useEffect @Maastrich?

[maastrich]

@lucabergamini yes

[rwilliams3088]

I ran into this same issue today - and since it was causing React rendering errors in the console, I decided to go ahead and put together a fix: https://github.com/remix-run/react-router/pull/10740

[finnhvman]

I ran into this same issue today - and since it was causing React rendering errors in the console, I decided to go ahead and put together a fix: #10740

Thank you! Hope to see this merged soon!

I ran into the issue today, absolutely unexpected and seems very dangerous.

[tengl]

What is the progress on this? Will the fix be merged?

[helguita]

I ran into this same issue today as well.

[kambing86]

it seems like a big foot gun, I noticed a bug in my app when calling setSearchParams one after another even I'm using the callback, hoping that it will work the same as setState in useState

[finnhvman]

any update? or does anyone know of an alternative to react-router which has a stable solution?

[flo5324]

I encountered the same problem recently, I always assumed that the setter was stable I have tested the fix by @rwilliams3088 #10740 and it seems to work like a charm Hope the PR will be reviewed as soon as possible

[holaChaitanya]

Guys, do you have any update on the issue? Had to disable the exhaustive deps check to ship the code!

[KurtGokhan]

Currently I see two issues with setSearchParams.

• It is not stable
• It doesn't pass the latest value in its callback function (see @maastrich's comment). This can be worked-around with flushSync but that is a waste of renders.

Setters are usually imperative, not reactive. That is why React's useState is a stable function and it allows us to pass a callback to ensure we are working with the latest data.

[alesmenzel]

Hi, this is still an issue @kentcdodds any plans to actually change this behaviour?

[kentcdodds]

I'm not on the team so I don't know

[alesmenzel]

@ryanflorence @mjackson There have been already several attempts to fix the behaviour over more than a year now without success

• https://github.com/remix-run/react-router/issues/9304
• https://github.com/remix-run/react-router/issues/9912
• https://github.com/remix-run/react-router/issues/8908 / https://github.com/remix-run/react-router/discussions/9851
• https://github.com/remix-run/react-router/issues/9991#is suecomment-1574119969
• https://github.com/remix-run/react-router/pull/10740

could you please state your reasons against merging #10740 so we can move forward.

[buzinas]

For those who need to fix this issue ASAP while we wait, we've created our own hook (drop-in replacement):

import {
  type SetStateAction,
  useCallback,
  useEffect,
  useMemo,
  useRef,
} from 'react'
import { useLocation, useNavigate } from '@remix-run/react' // switch to `react-router-dom` if using React Router

export default function useSearchParams() {
  const location = useLocation()
  const navigate = useNavigate()

  const searchParams = useMemo(
    () => new URLSearchParams(location.search),
    [location.search],
  )

  const searchParamsRef = useRef(searchParams)

  useEffect(() => {
    searchParamsRef.current = searchParams
  }, [searchParams])

  const setSearchParams = useCallback(
    (setter: SetStateAction<URLSearchParams>) => {
      const newParams =
        typeof setter === 'function' ? setter(searchParamsRef.current) : setter
      navigate(`?${newParams}`)
    },
    [navigate],
  )

  return [searchParams, setSearchParams] as const
}

[bdwain]

~any downside to doing this instead? I prefer it because it doesn't rely on a ref to pass the previous state to the setter function~ Edit: it doesn't work

export default function useSearchParams() {
  const location = useLocation();
  const navigate = useNavigate();

  const [searchParams, setSearchParams] = useState(
    () => new URLSearchParams(location.search),
  );

  const skipFirstNavigate = useRef<boolean>(false);

  useEffect(() => {
    if (!skipFirstNavigate.current) {
      skipFirstNavigate.current = true;
      return;
    }
    navigate(`?${searchParams}`);
  }, [navigate, searchParams]);

  return [searchParams, setSearchParams] as const;
}

[rwilliams3088]

any downside to doing this instead? I prefer it because it doesn't rely on a ref to pass the previous state to the setter function

You can download the project, make your modification, and try running the unit tests against it. It's been a minute - but if I recall correctly, setSearchParams allows you to perform partial updates to the parameters. IE, you don't need to pass all of the search params every time. Thus it is necessary to maintain a reference to the old search params in order to build the new search params - only overwriting the supplied values.

[bdwain]

ah i see. mine doesn't work with multiple params as the previous state only knows the the current params the last time that specific piece of state changed

any downside to doing this instead? I prefer it because it doesn't rely on a ref to pass the previous state to the setter function

You can download the project, make your modification, and try running the unit tests against it. It's been a minute - but if I recall correctly, setSearchParams allows you to perform partial updates to the parameters. IE, you don't need to pass all of the search params every time. Thus it is necessary to maintain a reference to the old search params in order to build the new search params - only overwriting the supplied values.

[bdwain]

@buzinas i had an issue with this version of the method when i made 2 changes to the url in quick succession in different hooks. They both acted off the same previous state.

The example where this occurred is having a perPage and offset value on a paginated page, and changing the perPage value causes offset to reset back to 0.

My solution was to move the reading of the current query variables as close as possible to the setting of the new ones, such that it's all synchronous, rather than trying to integrate it with the react lifecycle. This removes the option to pass the previous state to the setter, and requires you to pull document.location.search directly instead.

export function useSearchParams() {
  const location = useLocation();
  const navigate = useNavigate();

  const searchParams = useMemo(
    () => new URLSearchParams(location.search),
    [location.search],
  );

  const setSearchParams = useCallback(
    (getNewParams: () => URLSearchParams) => {
      navigate(`?${getNewParams()}`);
    },
    [navigate],
  );

  return [searchParams, setSearchParams] as const;

//example usage of setSearchParams
setSearchParams(() => {
  const searchParams = new URLSearchParams(document.location.search);
  searchParams.set('foo', 'bar');
  return searchParams;
});