-
-
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
Open
rwilliams3088
wants to merge
5
commits into
remix-run:dev
Choose a base branch
from
rwilliams3088:dev
base: dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
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 5409fab
Fix Lint Errors
rwilliams3088 bc016ce
Sign CLA
rwilliams3088 77fbc96
Code Review Changes; Use useEffect instead of useMemo for updating a …
rwilliams3088 b93f93d
Cleanup
rwilliams3088 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -179,6 +179,7 @@ | |
- rubeonline | ||
- ryanflorence | ||
- ryanhiebert | ||
- rwilliams3088 | ||
- sanketshah19 | ||
- scarf005 | ||
- senseibarni | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
: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 🙃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):
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.
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.
I think that should be alright, would just need to validate that it works
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)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 likeuseEffect
: https://react.dev/reference/react/useMemoAll 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 ofuseMemo
as nothing more than a specialized case ofuseEffect
that focuses on caching a value, with cache invalidation based upon the dependency list. Similarly,useCallback
is just a specialized case ofuseEffect
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.
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 ofuseMemo
. Notice that the execution ofnextCreate
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.
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:The
memoCount
can be observed to always update synchronously with therenderCount
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.
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.