-
Notifications
You must be signed in to change notification settings - Fork 329
fix(app-router): track hash-only traversal metadata #1306
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
Changes from all commits
146c100
bb8285a
8f87615
13cc466
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -226,6 +226,61 @@ function commitHistoryTraversalIndex(index: number | null): void { | |
| } | ||
| } | ||
|
|
||
| function commitHashOnlyNavigation( | ||
| href: string, | ||
| historyUpdateMode: Exclude<HistoryUpdateMode, undefined>, | ||
| scroll: boolean, | ||
| ): void { | ||
| const navigationHistoryIndex = allocateNavigationHistoryTraversalIndex(historyUpdateMode); | ||
| const previousNextUrl = hasBrowserRouterState() | ||
| ? getBrowserRouterState().previousNextUrl | ||
| : readHistoryStatePreviousNextUrl(window.history.state); | ||
| const historyState = createHistoryStateWithNavigationMetadata( | ||
| createHashOnlyNavigationBaseHistoryState(historyUpdateMode, scroll), | ||
| { | ||
| previousNextUrl, | ||
| traversalIndex: navigationHistoryIndex, | ||
| }, | ||
| ); | ||
|
|
||
| if (historyUpdateMode === "replace") { | ||
| replaceHistoryStateWithoutNotify(historyState, "", href); | ||
| } else { | ||
| pushHistoryStateWithoutNotify(historyState, "", href); | ||
| } | ||
| commitHistoryTraversalIndex(navigationHistoryIndex); | ||
| } | ||
|
|
||
| function createHashOnlyNavigationBaseHistoryState( | ||
| historyUpdateMode: Exclude<HistoryUpdateMode, undefined>, | ||
| scroll: boolean, | ||
| ): unknown { | ||
| if (historyUpdateMode !== "replace") { | ||
| return null; | ||
| } | ||
| return scroll ? stripVinextScrollState(window.history.state) : window.history.state; | ||
| } | ||
|
|
||
| function stripVinextScrollState(state: unknown): unknown { | ||
| if (!state || typeof state !== "object") { | ||
| return state; | ||
| } | ||
|
|
||
| const nextState: Record<string, unknown> = {}; | ||
| for (const [key, value] of Object.entries(state)) { | ||
| if (key === "__vinext_scrollX" || key === "__vinext_scrollY") { | ||
| continue; | ||
| } | ||
| nextState[key] = value; | ||
| } | ||
|
|
||
| return Object.keys(nextState).length > 0 ? nextState : null; | ||
| } | ||
|
|
||
| function commitTraversalIndexFromHistoryState(historyState: unknown): void { | ||
| commitHistoryTraversalIndex(readHistoryStateTraversalIndex(historyState)); | ||
| } | ||
|
|
||
| function getBrowserRouterState(): AppRouterState { | ||
| return browserNavigationController.getBrowserRouterState(); | ||
| } | ||
|
|
@@ -1113,6 +1168,7 @@ function bootstrapHydration(rscStream: ReadableStream<Uint8Array>): void { | |
| // header comment: "the segment cache contains the actual RSC data which | ||
| // needs to be re-fetched." | ||
| window.__VINEXT_CLEAR_NAV_CACHES__ = clearClientNavigationCaches; | ||
| window.__VINEXT_RSC_COMMIT_HASH_NAVIGATION__ = commitHashOnlyNavigation; | ||
|
|
||
| window.__VINEXT_RSC_NAVIGATE__ = async function navigateRsc( | ||
| href: string, | ||
|
|
@@ -1475,6 +1531,7 @@ function bootstrapHydration(rscStream: ReadableStream<Uint8Array>): void { | |
| const href = window.location.href; | ||
| if (isSameAppRoutePopstateTarget(href)) { | ||
| notifyAppRouterTransitionStart(href, "traverse"); | ||
| commitTraversalIndexFromHistoryState(event.state); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good addition. This is the same-route popstate fast path, so the traversal index must be committed here since |
||
| restorePopstateScrollPosition(event.state); | ||
| return; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| "use client"; | ||
|
|
||
| import { useRouter } from "next/navigation"; | ||
|
|
||
| export function HashActions() { | ||
| const router = useRouter(); | ||
|
|
||
| return ( | ||
| <> | ||
| <button id="replace-top" onClick={() => router.replace("#top")} type="button"> | ||
| Replace top | ||
| </button> | ||
| <button id="replace-content" onClick={() => router.replace("#content")} type="button"> | ||
| Replace content | ||
| </button> | ||
| </> | ||
| ); | ||
| } |
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.
Nit: when
stateis a non-null object whose only keys are__vinext_scrollXand__vinext_scrollY, this returnsnull. That means a replace-with-scroll of a history entry that had only vinext scroll keys (no__vinext_historyIndex, no__vinext_previousNextUrl) would produce anullbase state, whichcreateHistoryStateWithNavigationMetadatathen populates from scratch — so functionally correct. Just flagging the implicit contract in case a future caller passes state through this function outside ofcreateHashOnlyNavigationBaseHistoryState, where the metadata-writing step wouldn't necessarily follow.