Skip to content
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.

Sign up for GitHub

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

Jump to bottom

useSearchParams Bug Fix | Do not regenerate the setSearchParams function when the searchParams change #10740

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

useSearchParams Bug Fix | Do not regenerate the setSearchParams function when the searchParams change #10740

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
22d687e
Fix the useSearchParams() hook so that the setSearchParams function i…
rwilliams3088 Jul 27, 2023
5409fab
Fix Lint Errors
rwilliams3088 Jul 27, 2023
bc016ce
Sign CLA
rwilliams3088 Jul 27, 2023
77fbc96
Code Review Changes; Use useEffect instead of useMemo for updating a …
rwilliams3088 Dec 15, 2023
b93f93d
Cleanup
rwilliams3088 Dec 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions contributors.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@
- rubeonline
- ryanflorence
- ryanhiebert
- rwilliams3088
- sanketshah19
- scarf005
- senseibarni
Expand Down
108 changes: 108 additions & 0 deletions packages/react-router-dom/__tests__/search-params-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { waitFor } from "@testing-library/react";
import * as React from "react";
import * as ReactDOM from "react-dom/client";
import { act } from "react-dom/test-utils";
import type { SetURLSearchParams } from "react-router-dom";
import { MemoryRouter, Routes, Route, useSearchParams } from "react-router-dom";

describe("useSearchParams", () => {
Expand Down Expand Up @@ -182,4 +184,110 @@ describe("useSearchParams", () => {
`"<p>value=initial&amp;a=1&amp;b=2</p>"`
);
});

it("does not modify the setSearchParams reference when the searchParams change", async () => {
interface TestParams {
incrementParamsUpdateCount: () => number;
incrementSetterUpdateCount: () => number;
}

function TestComponent(params: Readonly<TestParams>) {
const { incrementParamsUpdateCount, incrementSetterUpdateCount } = params;
const lastParamsRef = React.useRef<URLSearchParams>();
const lastSetterRef = React.useRef<SetURLSearchParams>();

const queryRef = React.useRef<HTMLInputElement>(null);
const [searchParams, setSearchParams] = useSearchParams({ q: "" });
const query = searchParams.get("q")!;

if(lastParamsRef.current !== searchParams) {
incrementParamsUpdateCount();
}
lastParamsRef.current = searchParams;

if(lastSetterRef.current !== setSearchParams) {
incrementSetterUpdateCount();
}
lastSetterRef.current = setSearchParams;

function handleSubmit(event: React.FormEvent<HTMLFormElement>) {
event.preventDefault();
if (queryRef.current) {
setSearchParams({ q: queryRef.current.value });
}
}

return (
<div>
<p>The current query is "{query}".</p>
<form onSubmit={handleSubmit}>
<input name="q" defaultValue={query} ref={queryRef} />
</form>
</div>
);
}

function TestApp(params: TestParams) {
return (
<MemoryRouter initialEntries={["/search?q=Something"]}>
<Routes>
<Route path="search" element={<TestComponent {...params} />} />
</Routes>
</MemoryRouter>
);
}

const state = {
paramsUpdateCount: 0,
setterUpdateCount: 0
};

const params: TestParams = {
incrementParamsUpdateCount: () => ++state.paramsUpdateCount,
incrementSetterUpdateCount: () => ++state.setterUpdateCount
};

// Initial Rendering of the TestApp
// The TestComponent should increment both the paramsUpdateCount and setterUpdateCount to 1
act(() => {
ReactDOM.createRoot(node).render(<TestApp {...params} />);
});

let form = node.querySelector("form")!;
let queryInput = node.querySelector<HTMLInputElement>("input[name=q]")!;
await waitFor(() => {
expect(form).toBeDefined();
expect(queryInput).toBeDefined();
expect(state.paramsUpdateCount).toEqual(1);
expect(state.setterUpdateCount).toEqual(1);
});

// Modify the search params via the form in the TestComponent.
// This should trigger a re-render of the component and update the paramsUpdateCount (only)
act(() => {
queryInput.value = "Something+Else";
form.dispatchEvent(
new Event("submit", { bubbles: true, cancelable: true })
);
});

await waitFor(() => {
expect(state.paramsUpdateCount).toEqual(2);
expect(state.setterUpdateCount).toEqual(1);
});

// Third Times The Charm
// Verifies that the setter is still valid
act(() => {
queryInput.value = "on+a+boat";
form.dispatchEvent(
new Event("submit", { bubbles: true, cancelable: true })
);
});

await waitFor(() => {
expect(state.paramsUpdateCount).toEqual(3);
expect(state.setterUpdateCount).toEqual(1);
});
});
});
13 changes: 8 additions & 5 deletions packages/react-router-dom/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -993,30 +993,33 @@ export function useSearchParams(

let defaultSearchParamsRef = React.useRef(createSearchParams(defaultInit));
let hasSetSearchParamsRef = React.useRef(false);
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]
Copy link

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 🙃

Copy link

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]
);

Copy link
Author

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 🙃

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

Copy link

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)

Copy link
Author

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 in useMemo since the callback of a useMemo 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.

Copy link

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.

image

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.

Copy link

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..

Copy link
Author

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.

Copy link

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.

Copy link
Author

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.

);

let navigate = useNavigate();
let setSearchParams = React.useCallback<SetURLSearchParams>(
(nextInit, navigateOptions) => {
const newSearchParams = createSearchParams(
typeof nextInit === "function" ? nextInit(searchParams) : nextInit
typeof nextInit === "function" ? nextInit(searchParamsRef.current) : nextInit
);
hasSetSearchParamsRef.current = true;
navigate("?" + newSearchParams, navigateOptions);
},
[navigate, searchParams]
[navigate]
);

return [searchParams, setSearchParams];
Expand Down
94 changes: 93 additions & 1 deletion packages/react-router-native/__tests__/search-params-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import * as React from "react";
import { View, Text, TextInput } from "react-native";
import type {
SetURLSearchParams} from "react-router-native";
import {
NativeRouter,
Routes,
Route,
useSearchParams,
useSearchParams
} from "react-router-native";
import * as TestRenderer from "react-test-renderer";

Expand Down Expand Up @@ -152,4 +154,94 @@ describe("useSearchParams", () => {

expect(renderer.toJSON()).toMatchSnapshot();
});

it("does not modify the setSearchParams reference when the searchParams change", async () => {
interface TestParams {
incrementParamsUpdateCount: () => number;
incrementSetterUpdateCount: () => number;
}

function TestComponent(params: Readonly<TestParams>) {
const { incrementParamsUpdateCount, incrementSetterUpdateCount } = params;
const lastParamsRef = React.useRef<URLSearchParams>();
const lastSetterRef = React.useRef<SetURLSearchParams>();
const [searchParams, setSearchParams] = useSearchParams({ q: "" });
const [query] = React.useState(searchParams.get("q")!);

if(lastParamsRef.current !== searchParams) {
incrementParamsUpdateCount();
}
lastParamsRef.current = searchParams;

if(lastSetterRef.current !== setSearchParams) {
incrementSetterUpdateCount();
}
lastSetterRef.current = setSearchParams;

function handleSubmit() {
setSearchParams(cur => {
cur.set("q", `${cur.get("q")} - appended`);
return cur;
});
}

return (
<View>
<SearchForm onSubmit={handleSubmit}>
<TextInput value={query} /*onChangeText={setQuery}*/ />
</SearchForm>
</View>
);
}

function TestApp(params: TestParams) {
return (
<NativeRouter initialEntries={["/search?q=Michael+Jackson"]}>
<Routes>
<Route path="search" element={<TestComponent {...params} />} />
</Routes>
</NativeRouter>
);
}

const state = {
paramsUpdateCount: 0,
setterUpdateCount: 0
};

const params: TestParams = {
incrementParamsUpdateCount: () => ++state.paramsUpdateCount,
incrementSetterUpdateCount: () => ++state.setterUpdateCount
};

// Initial Rendering of the TestApp
// The TestComponent should increment both the paramsUpdateCount and setterUpdateCount to 1
let renderer: TestRenderer.ReactTestRenderer;
await TestRenderer.act(() => {
renderer = TestRenderer.create(<TestApp {...params}/>);
});

expect(state.paramsUpdateCount).toEqual(1);
expect(state.setterUpdateCount).toEqual(1);

// Modify the search params via the form in the TestComponent.
// This should trigger a re-render of the component and update the paramsUpdateCount (only)
const searchForm = renderer!.root.findByType(SearchForm);
await TestRenderer.act(() => {
searchForm.props.onSubmit();
});

expect(state.paramsUpdateCount).toEqual(2);
expect(state.setterUpdateCount).toEqual(1);

// Third Times The Charm
// Verifies that the setter is still valid now that we aren't regenerating each time the
// searchParams reference changes
await TestRenderer.act(() => {
searchForm.props.onSubmit();
});

expect(state.paramsUpdateCount).toEqual(3);
expect(state.setterUpdateCount).toEqual(1);
});
});
6 changes: 4 additions & 2 deletions packages/react-router-native/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,7 @@ export function useSearchParams(
): [URLSearchParams, SetURLSearchParams] {
let defaultSearchParamsRef = React.useRef(createSearchParams(defaultInit));
let hasSetSearchParamsRef = React.useRef(false);
let searchParamsRef = React.useRef<URLSearchParams>(defaultSearchParamsRef.current);

let location = useLocation();
let searchParams = React.useMemo(() => {
Expand All @@ -305,19 +306,20 @@ export function useSearchParams(
}
}

searchParamsRef.current = searchParams;
return searchParams;
}, [location.search]);

let navigate = useNavigate();
let setSearchParams = React.useCallback<SetURLSearchParams>(
(nextInit, navigateOpts) => {
const newSearchParams = createSearchParams(
typeof nextInit === "function" ? nextInit(searchParams) : nextInit
typeof nextInit === "function" ? nextInit(searchParamsRef.current) : nextInit
);
hasSetSearchParamsRef.current = true;
navigate("?" + newSearchParams, navigateOpts);
},
[navigate, searchParams]
[navigate]
);

return [searchParams, setSearchParams];
Expand Down