-
Notifications
You must be signed in to change notification settings - Fork 325
fix(app-router): preserve history metadata for external state updates #1259
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
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 |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| const VINEXT_PREVIOUS_NEXT_URL_HISTORY_STATE_KEY = "__vinext_previousNextUrl"; | ||
|
|
||
| type HistoryStateRecord = { | ||
| [key: string]: unknown; | ||
| }; | ||
|
|
||
| function cloneHistoryState(state: unknown): HistoryStateRecord { | ||
| if (!state || typeof state !== "object") { | ||
| return {}; | ||
| } | ||
|
|
||
| const nextState: HistoryStateRecord = {}; | ||
| for (const [key, value] of Object.entries(state)) { | ||
| nextState[key] = value; | ||
| } | ||
| return nextState; | ||
| } | ||
|
|
||
| export function createHistoryStateWithPreviousNextUrl( | ||
| state: unknown, | ||
| previousNextUrl: string | null, | ||
| ): HistoryStateRecord | null { | ||
| const nextState = cloneHistoryState(state); | ||
|
|
||
| if (previousNextUrl === null) { | ||
| delete nextState[VINEXT_PREVIOUS_NEXT_URL_HISTORY_STATE_KEY]; | ||
| } else { | ||
| nextState[VINEXT_PREVIOUS_NEXT_URL_HISTORY_STATE_KEY] = previousNextUrl; | ||
| } | ||
|
|
||
| return Object.keys(nextState).length > 0 ? nextState : null; | ||
| } | ||
|
|
||
| export function createExternalHistoryStatePreservingMetadata( | ||
| callerState: unknown, | ||
| currentHistoryState: unknown, | ||
| ): unknown { | ||
| const previousNextUrl = readHistoryStatePreviousNextUrl(currentHistoryState); | ||
| if (previousNextUrl === null) { | ||
| return callerState; | ||
| } | ||
|
|
||
| return createHistoryStateWithPreviousNextUrl(callerState, previousNextUrl); | ||
| } | ||
|
|
||
| export function readHistoryStatePreviousNextUrl(state: unknown): string | null { | ||
| const value = cloneHistoryState(state)[VINEXT_PREVIOUS_NEXT_URL_HISTORY_STATE_KEY]; | ||
| return typeof value === "string" ? value : null; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -225,6 +225,80 @@ describe("next/navigation shim", () => { | |
| } | ||
| }); | ||
|
|
||
| it("preserves App Router history metadata when external history calls provide caller state", async () => { | ||
| // Matches Next.js' external History API wrapper behavior: | ||
| // https://github.com/vercel/next.js/blob/canary/packages/next/src/client/components/app-router.tsx#L114-L127 | ||
| // Covered by Next.js shallow-routing tests for object, null, and undefined state: | ||
| // https://github.com/vercel/next.js/blob/canary/test/e2e/app-dir/shallow-routing/shallow-routing.test.ts | ||
| const previousWindow = (globalThis as any).window; | ||
| const historyMetadataKey = "__vinext_previousNextUrl"; | ||
| const win = { | ||
| location: { | ||
| pathname: "/photo/1", | ||
| search: "", | ||
| hash: "", | ||
| href: "http://localhost/photo/1", | ||
| origin: "http://localhost", | ||
| }, | ||
| history: { | ||
| state: { [historyMetadataKey]: "/feed" } as unknown, | ||
| pushState(data: unknown, _unused: string, url?: string | URL | null) { | ||
| this.state = data; | ||
| if (!url) return; | ||
| const parsed = new URL(url, win.location.href); | ||
| win.location.pathname = parsed.pathname; | ||
| win.location.search = parsed.search; | ||
| win.location.hash = parsed.hash; | ||
| win.location.href = parsed.href; | ||
| }, | ||
| replaceState(data: unknown, _unused: string, url?: string | URL | null) { | ||
| this.state = data; | ||
| if (!url) return; | ||
| const parsed = new URL(url, win.location.href); | ||
| win.location.pathname = parsed.pathname; | ||
| win.location.search = parsed.search; | ||
| win.location.hash = parsed.hash; | ||
| win.location.href = parsed.href; | ||
| }, | ||
| }, | ||
| addEventListener: vi.fn(), | ||
| }; | ||
| (globalThis as any).window = win; | ||
|
|
||
| try { | ||
| vi.resetModules(); | ||
| await import("../packages/vinext/src/shims/navigation.js"); | ||
|
|
||
| win.history.pushState({ myData: { foo: "bar" } }, "", "/photo/1?filter=active"); | ||
|
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. Consider adding a test case where the current history state does not have For example: win.history.state = { unrelated: true };
win.history.pushState({ myData: 1 }, "", "/photo/2");
expect(win.history.state).toEqual({ myData: 1 }); // no metadata injectedNot blocking — the unit under test (
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. Also worth considering: a case where the caller's own state object contains |
||
| expect(win.history.state).toEqual({ | ||
| myData: { foo: "bar" }, | ||
| [historyMetadataKey]: "/feed", | ||
| }); | ||
|
|
||
| win.history.pushState(null, "", "/photo/1?filter=pending"); | ||
| expect(win.history.state).toEqual({ | ||
| [historyMetadataKey]: "/feed", | ||
| }); | ||
|
|
||
| win.history.replaceState(null, "", "/photo/1?filter=archived"); | ||
| expect(win.history.state).toEqual({ | ||
| [historyMetadataKey]: "/feed", | ||
| }); | ||
|
|
||
| win.history.replaceState(undefined, "", "/photo/1?filter=all"); | ||
| expect(win.history.state).toEqual({ | ||
| [historyMetadataKey]: "/feed", | ||
| }); | ||
| } finally { | ||
| vi.resetModules(); | ||
| if (previousWindow === undefined) { | ||
| delete (globalThis as any).window; | ||
| } else { | ||
| (globalThis as any).window = previousWindow; | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| it("exports redirect, notFound, permanentRedirect", async () => { | ||
| const nav = await import("../packages/vinext/src/shims/navigation.js"); | ||
| expect(typeof nav.redirect).toBe("function"); | ||
|
|
||
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:
createHistoryStateWithPreviousNextUrlreturnsHistoryStateRecord | null, butcreateExternalHistoryStatePreservingMetadatais typed as returningunknown. WhenpreviousNextUrlis non-null andcallerStateisnull, the returned value is{ __vinext_previousNextUrl: "/feed" }— which is fine — but on thecallerStatepassthrough path (line 40), the return type isunknownbecause the caller's original value is returned as-is.This works correctly at runtime, but the return type could be narrowed. Not blocking.