Implement segment-scoped useRouter bfcacheId semantics#1197
Conversation
commit: |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Segment-scoped useRouter().bfcacheId
Overall this is a well-structured implementation. The segment identity model (tying bfcacheIds to path-prefix equality for layouts and full-pathname equality for pages), history state persistence for traversals, and the two-context approach (BfcacheIdMapContext + BfcacheSegmentIdContext) are all sound. Test coverage is thorough — both unit tests for the state helpers and E2E tests ported from the upstream Next.js commit.
A few items worth discussing, ranging from a potential memory leak to smaller correctness/style points:
| }, | ||
| }; | ||
|
|
||
| const _appRouterByBfcacheId = new Map<string, typeof _appRouter>(); |
There was a problem hiding this comment.
Unbounded cache growth. _appRouterByBfcacheId is a Map that grows one entry per unique bfcacheId over the session lifetime but is never pruned. Every fresh navigation mints a new id (e.g. _b_1_, _b_2_, ...) and each produces a cached router object here that is never evicted.
In practice the objects are tiny (shallow spread of _appRouter + one string), so this won't cause real memory pressure in any realistic session. But it's still a leak by principle — and if someone instruments the router object (attaches large debug data, etc.), it could compound.
Consider either:
- Switching to a
WeakRef-based cache or an LRU with a small bound (e.g. 64 entries) - Clearing the map inside
clearClientNavigationCaches()(called byrouter.refresh()) - Documenting that the cache is intentionally unbounded and considered acceptable
Not a blocker, but worth a one-line comment at minimum explaining the decision.
| | DispatchPendingNavigationCommitDispositionDecision | ||
| | NonDispatchPendingNavigationCommitDispositionDecision; | ||
|
|
||
| let nextBfcacheId = 0; |
There was a problem hiding this comment.
Module-level mutable counter is shared across test runs. nextBfcacheId is a module-level let that only ever increases (via rememberBfcacheId / mintBfcacheId). Because Vitest reuses the module between tests in the same file, the counter carries forward. This means tests can't assert exact id values — they must use regex patterns like /^_b_\d+_$/.
The tests already do this, so it works today. But it's fragile: a test that calls mintBfcacheId (indirectly through createNextBfcacheIdMap) will bump the counter, affecting subsequent tests' id values. If a future test needs to assert exact ids (e.g. for snapshot testing), it'll be surprised.
Consider exporting a resetBfcacheIdCounter() test helper (guarded behind process.env.NODE_ENV !== 'production' or only exported from the test barrel), or restructuring the counter to be injectable.
| const currentIdentity = createBfcacheSegmentIdentity(id, options.currentPathname); | ||
| const nextIdentity = createBfcacheSegmentIdentity(id, options.nextPathname); | ||
| const currentValue = currentIdentity === nextIdentity ? options.current[id] : undefined; | ||
| const value = options.restored?.[id] ?? currentValue ?? mintBfcacheId(); |
There was a problem hiding this comment.
Subtle ordering question: restored takes priority over currentValue, which makes sense for traversals (back/forward should restore the exact prior ids). But what happens if the restored map has stale segment ids that no longer exist in elements?
The for...of collectBfcacheSegmentIds(options.elements) loop ensures we only produce ids for segments present in the new elements, so stale keys from restored are silently dropped. That's correct behavior. Just confirming this is intentional — worth a brief inline comment explaining the precedence: restored > current-if-identity-matches > mint-fresh.
| return router; | ||
| } | ||
|
|
||
| function hasReactDispatcher(): boolean { |
There was a problem hiding this comment.
Accessing React internals. __CLIENT_INTERNALS_DO_NOT_USE_OR_WARN_USERS_THEY_CANNOT_UPGRADE is a React implementation detail that could change across React versions. The function name itself is a warning.
I understand the motivation: useContext can only be called inside a React component/hook call stack, so you need a way to detect whether a dispatcher is active before calling it. This is the same pattern Next.js uses internally.
Two concerns:
- If React changes this internal (they've renamed it before — it was
__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIREDin React 18), this will silently fall back to"0"instead of crashing, which is the safe direction. Good. - The
try/catchinreadBfcacheIdFromContextalready handles the no-dispatcher case. Is thehasReactDispatcher()pre-check actually needed, or could you rely solely on the catch? IfuseContextthrows without a dispatcher, the catch returns"0"anyway.
If the pre-check is a performance optimization (avoiding the exception cost on hot paths outside React), that's reasonable — but worth a comment explaining why both guards exist.
| replaceHistoryStateWithoutNotify(historyState, "", href); | ||
| } else if (historyUpdateMode === "push" && window.location.href !== targetHref) { | ||
| pushHistoryStateWithoutNotify(historyState, "", href); | ||
| } else if (historyUpdateMode === undefined) { |
There was a problem hiding this comment.
This new else if arm fires for traversals (historyUpdateMode === undefined) to persist the updated bfcacheIds into history.state even when the URL hasn't changed. Makes sense — but it also fires for refresh navigations (which also pass undefined for historyUpdateMode).
For refresh, this replaceState is harmless (same URL, same-ish state), but it's a behavioral change from before this PR where refresh commits didn't touch history state at all. Worth a comment clarifying that this is intentional for both traverse and refresh cases.
| const hash = fullHref.includes("#") ? fullHref.slice(fullHref.indexOf("#")) : ""; | ||
| if (mode === "replace") { | ||
| replaceHistoryStateWithoutNotify(null, "", fullHref); | ||
| replaceHistoryStateWithoutNotify(window.history.state, "", fullHref); |
There was a problem hiding this comment.
Good fix: preserving window.history.state through hash-only navigations ensures bfcacheIds (and other vinext state like __vinext_previousNextUrl, __vinext_scrollY) survive hash changes. The old code passed null, which would have wiped all history state on hash-only navigation. This was a pre-existing bug that this PR correctly fixes as a side effect of the bfcacheId work.
Worth noting in the PR description that this is a broader fix beyond just bfcacheId.
| } | ||
|
|
||
| return ( | ||
| const BfcacheSegmentIdContext = getBfcacheSegmentIdContext(); |
There was a problem hiding this comment.
Calling getBfcacheSegmentIdContext() on every Slot render creates/fetches the context via the global symbol lookup. This is fast (global property access + null check), but it's called once per slot per render. Since the result is stable (created once, then cached on globalThis), consider hoisting this to module level like ServerInsertedHTMLContext is in navigation.ts — unless there's a reason it needs to be lazily initialized per render (e.g., RSC environment where createContext isn't available).
Actually, looking at this more carefully: slot.tsx is marked "use client", so createContext is always available here. You could safely hoist to module level:
| const BfcacheSegmentIdContext = getBfcacheSegmentIdContext(); | |
| const BfcacheSegmentIdContext = getBfcacheSegmentIdContext()!; |
Or compute it once at module scope. Same applies to the getBfcacheIdMapContext() calls in app-browser-entry.ts and app-ssr-entry.ts that are inside render functions.
| for (const layoutId of AppElementsWire.readMetadata(elements).layoutIds) { | ||
| ids.add(layoutId); | ||
| } | ||
| } catch { |
There was a problem hiding this comment.
Silencing the catch with an empty block is fine for test robustness, but consider at least a // ignore or more specific comment about which tests hit this path, so future readers don't wonder if this swallows real errors in production.
| @@ -0,0 +1,97 @@ | |||
| /** | |||
| * Next.js Compat E2E: useRouter().bfcacheId | |||
| * Ported from: https://github.com/vercel/next.js/blob/56d95137fd6d84f4bc1e5ef2bb31e0136d5fad9c/test/e2e/app-dir/use-router-bfcache-id/use-router-bfcache-id.test.ts | |||
There was a problem hiding this comment.
Nice to see the upstream commit reference. The E2E tests cover the key scenarios well: fresh minting, traversal restoration, form state reset via key={bfcacheId}, shared layout preservation, cross-group reset, and hash/search/refresh stability.
One scenario not covered: programmatic navigation via router.push/router.replace (as opposed to <Link> clicks). The tests use revealAndClick which goes through <Link>. Consider adding a test that calls router.push() directly (e.g., via a button onClick) to confirm bfcacheId minting works the same way for programmatic navigations.
|
Review posted. Here's a summary of the feedback: Architecture is solid. The two-context approach ( Key feedback points:
|
|
@james-elicx Thanks for the review. I pushed a follow-up commit addressing the concrete items and added comments where the behavior is intentional. Could you take another look when you have a chance? |
|
/bigbonk review |
There was a problem hiding this comment.
Re-review: Segment-scoped useRouter().bfcacheId
The follow-up commit addresses all the concrete items from the first review cleanly:
- Unbounded cache → Fixed with an LRU cap of 64 entries plus MRU promotion on hit. Good.
hasReactDispatcher()removed → The__CLIENT_INTERNALS_DO_NOT_USE_OR_WARN_USERS_THEY_CANNOT_UPGRADEprobe is gone; thetry/catchonuseContextis the sole guard now. Simpler and more resilient to React internals changes.BfcacheIdMapContexthoisted to module scope → Done inapp-browser-entry.ts,app-ssr-entry.ts, andslot.tsx. All three now resolve the context once at module load instead of per-render.- Traversal/refresh
replaceStatecommented → Theelse if (historyUpdateMode === undefined)arm now has an inline comment explaining it covers both traversals and refreshes. - Programmatic navigation E2E → Added a test for
router.push()androuter.replace()with fixture buttons. Covers the gap noted in the first review. INITIAL_BFCACHE_IDconstant → Magic"0"string extracted to a named constant.- Precedence comment in
createNextBfcacheIdMap→ Added inline explaining restored > current-if-identity-matches > mint-fresh.
CI is fully green (all Vitest shards, all 11 Playwright E2E projects, Check, create-next-app).
Two minor observations below — neither is blocking. This is ready to merge.
| // their current id, and newly-created segments mint a fresh opaque id. | ||
| const value = options.restored?.[id] ?? currentValue ?? mintBfcacheId(); | ||
| ids[id] = value; | ||
| rememberBfcacheId(value); |
There was a problem hiding this comment.
Nit: this rememberBfcacheId(value) call is redundant. By this point value is one of:
- From
options.restored— already remembered at line 260–261 - From
options.current— already remembered at line 257–258 - From
mintBfcacheId()— freshly minted with++nextBfcacheId, sorememberBfcacheIdis a no-op (the counter is already at or past this value)
Not harmful (it's a fast regex + Math.max no-op), but removing it would make the data flow clearer. Totally optional.
| ); | ||
| } | ||
|
|
||
| function createHashOnlyHistoryState(state: unknown): unknown { |
There was a problem hiding this comment.
Nice addition — the allowlist approach (only carrying __vinext_previousNextUrl and __vinext_bfcacheIds forward through hash navigations) is correct. It intentionally drops scroll position keys and any third-party state that might have been on the history entry.
Worth noting: if vinext ever adds more keys to history state (beyond the current four), this function will need updating too. A quick future-proofing option would be to define the set of VINEXT_*_HISTORY_STATE_KEY constants in one place and have this function iterate over a shared list, but that's a refinement for another day.
|
Review posted. Here's a summary: Approved. The follow-up commit cleanly addresses all 7 concrete items from the first review:
CI is fully green across all checks. Left two non-blocking nits: a redundant |
2 participants
Summary
useRouter().bfcacheIdvalues for the App Router browser state56d9513This is a follow-up to the placeholder API-surface PR. It keeps the public field added by #1192, but wires it into vinext's current App Router state model.
Tests
pnpm exec vp check packages/vinext/src/server/app-browser-state.ts packages/vinext/src/server/app-browser-visible-commit.ts packages/vinext/src/server/app-browser-navigation-controller.ts packages/vinext/src/server/app-browser-entry.ts packages/vinext/src/server/app-ssr-entry.ts packages/vinext/src/shims/navigation.ts packages/vinext/src/shims/slot.tsx tests/app-browser-entry.test.ts tests/shims.test.ts tests/e2e/app-router/nextjs-compat/use-router-bfcache-id.spec.tspnpm exec vp test run tests/shims.test.ts -t "useRouter"pnpm exec vp test run tests/app-browser-entry.test.ts -t "bfcacheId"pnpm exec vp run vinext#buildPLAYWRIGHT_PROJECT=app-router pnpm exec playwright test tests/e2e/app-router/nextjs-compat/use-router-bfcache-id.spec.ts