fix: make useParams() reactive on client-side navigation#489
fix: make useParams() reactive on client-side navigation#489james-elicx merged 3 commits intocloudflare:mainfrom
Conversation
useParams() was reading a module-level variable directly instead of subscribing via useSyncExternalStore, so navigating between dynamic routes (e.g. /posts/1 → /posts/2) wouldn't re-render components. Two changes: - useParams() now uses useSyncExternalStore with the shared listener system, matching usePathname() and useSearchParams() - setClientParams() now calls notifyListeners() when params change, so the external store knows to re-check snapshots
commit: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88e58f6c2c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }; | ||
| }, | ||
| () => _clientParams as T, | ||
| () => (_getServerContext()?.params ?? {}) as T, |
There was a problem hiding this comment.
Return a stable empty snapshot in useParams
When _getServerContext() is null (the file already notes this can happen during SSR/hydration fallback paths), this getServerSnapshot callback creates a fresh {} each call. useSyncExternalStore relies on snapshot identity stability during hydration, so this can cause hydration mismatch churn/warnings and extra renders for useParams() consumers whenever nav context is missing. Use a cached module-level empty params object for the fallback, similar to the cached empty server search params path.
Useful? React with 👍 / 👎.
|
/bigbonk review |
There was a problem hiding this comment.
Good fix — useParams() absolutely needs useSyncExternalStore to be reactive on client-side navigation, matching the existing usePathname() and useSearchParams() patterns. The test coverage is solid (forward nav, multi-hop, back/forward). Two issues below, one of which can cause real bugs.
| }; | ||
| }, | ||
| () => _clientParams as T, | ||
| () => (_getServerContext()?.params ?? {}) as T, |
There was a problem hiding this comment.
Bug: unstable getServerSnapshot will cause hydration churn
When _getServerContext() returns null (which the comment on line 373 says can happen), this creates a fresh {} on every call. useSyncExternalStore compares snapshots with Object.is, so two {} objects are never equal → infinite re-render loop during SSR hydration fallback.
The existing useSearchParams hook handles this exact pattern with a cached empty fallback (_cachedEmptyServerSearchParams at line 261). useParams should do the same:
| () => (_getServerContext()?.params ?? {}) as T, | |
| () => (_getServerContext()?.params ?? _EMPTY_PARAMS) as T, |
With a module-level constant:
const _EMPTY_PARAMS: Record<string, string | string[]> = {};Place it near line 300 alongside the other cached client params state.
| _listeners.delete(cb); | ||
| }; | ||
| }, | ||
| () => _clientParams as T, |
There was a problem hiding this comment.
Nit: extract snapshot to a named function for consistency
Both usePathname() and useSearchParams() use named module-level snapshot functions (getPathnameSnapshot, getSearchParamsSnapshot) instead of inline arrows. This avoids creating a new closure on every render call. It doesn't affect correctness (React stores the latest snapshot function), but it's more consistent with the existing code style and marginally cheaper.
| () => _clientParams as T, | |
| getClientParamsSnapshot as () => T, |
With:
function getClientParamsSnapshot(): Record<string, string | string[]> {
return _clientParams;
}| return React.useSyncExternalStore( | ||
| (cb: () => void) => { | ||
| _listeners.add(cb); | ||
| return () => { | ||
| _listeners.delete(cb); | ||
| }; |
There was a problem hiding this comment.
Nit: repeated subscribe boilerplate
This is the third copy of the identical subscribe function (also in usePathname at line 334 and useSearchParams at line 355). Not a blocker for this PR, but a follow-up extracting a shared subscribeToNavigation helper would clean things up:
function subscribeToNavigation(cb: () => void): () => void {
_listeners.add(cb);
return () => { _listeners.delete(cb); };
}|
Review submitted. Here's a summary of the feedback: Overall: Good fix. One bug to fix:
Two style nits:
|
- Add _EMPTY_PARAMS constant to avoid creating a new {} on every
getServerSnapshot call (Object.is would never match, causing
hydration churn)
- Extract getClientParamsSnapshot/getServerParamsSnapshot named
functions for consistency with usePathname/useSearchParams
- Extract subscribeToNavigation helper to deduplicate the identical
subscribe callback across all three hooks
|
/bigbonk review |
There was a problem hiding this comment.
This is a clean, correct fix. The bug diagnosis is accurate — useParams() was reading _clientParams directly without useSyncExternalStore, so client-side navigations between dynamic routes silently failed to trigger re-renders.
The implementation follows the established patterns from usePathname() and useSearchParams() exactly:
useSyncExternalStorewith a named snapshot function (getClientParamsSnapshot)- Stable
_EMPTY_PARAMSmodule-level constant for the server snapshot fallback (avoiding the hydration churn bug from() => ({})) notifyListeners()insetClientParams()only when params actually change (leveraging the existing JSON comparison guard)- Shared
subscribeToNavigationhelper extracted and used across all three hooks
All three issues from the earlier reviews (unstable server snapshot, inline snapshot arrow, duplicated subscribe boilerplate) are addressed in this revision.
E2E tests cover the right scenarios: forward navigation, multi-hop chains, and browser history back/forward. The fixture page is minimal and focused.
LGTM.
|
Review submitted and approved. The PR is a clean, correct fix for |
Summary
useParams()was reading a module-level variable directly instead of subscribing viauseSyncExternalStore, so navigating between dynamic routes (e.g./posts/1→/posts/2) wouldn't re-render components usinguseParams()useParams()now usesuseSyncExternalStorewith the shared listener system, matchingusePathname()anduseSearchParams()setClientParams()now callsnotifyListeners()when params actually change, so the external store re-checks snapshotsTest plan
useParamsupdates reactively on client-side Link navigation (/1→/2)useParamsupdates across multiple consecutive navigations (/1→/2→/3→/1)useParamsupdates on browser back/forward history navigation