fix: expose window.next global for Next.js parity#1201
Conversation
Next.js publishes window.next from both the Pages Router client bootstrap
(packages/next/src/client/next.ts) and the App Router bootstrap
(packages/next/src/client/app-bootstrap.ts). Userland code and the Next.js
deploy test suite read window.next.router directly (push, replace,
events.on, etc.), and Next.js's own client code reads
window.next.__pendingUrl and window.next.__internal_src_page for App
Router navigation bookkeeping.
vinext did not install this global, which surfaced as ~422 console errors
and 30+ runtime failures in the Next.js deploy test suite, with
"TypeError: Cannot read properties of undefined (reading 'router')" the
most cited.
Adds an installWindowNext helper in client/window-next.ts that both router
bootstraps invoke:
- Pages Router (shims/router.ts) installs window.next.router pointing at
the Router singleton at module load.
- App Router (server/app-browser-entry.ts) installs
window.next = { appDir: true, router: appRouterInstance } before
hydration starts, mirroring app-bootstrap.ts.
The helper merges fields on repeat calls so hybrid Pages+App setups
behave like Next.js (the App Router bootstrap wins for router/appDir,
matching Next.js's load order). __pendingUrl and __internal_src_page
remain undefined until the navigation paths populate them; that matches
Next.js's behavior on initial load.
|
/bigbonk review |
commit: |
|
| // Declare the `next` property on Window here, alongside the type, so this | ||
| // module type-checks standalone without depending on the global.d.ts | ||
| // augmentation (which itself imports WindowNext from this file). Matches the | ||
| // pattern Next.js uses in `packages/next/src/client/next.ts` lines 7-11: | ||
| // declare global { interface Window { next: any } } | ||
| declare global { | ||
| // oxlint-disable-next-line typescript/consistent-type-definitions | ||
| interface Window { | ||
| next?: WindowNext; | ||
| } | ||
| } |
There was a problem hiding this comment.
can this be moved to the global type decls file potentially?
| export type AppRouterPublicInstance = { | ||
| push: (href: string, options?: { scroll?: boolean }) => void; | ||
| replace: (href: string, options?: { scroll?: boolean }) => void; | ||
| back: () => void; | ||
| forward: () => void; | ||
| refresh: () => void; | ||
| prefetch: (href: string) => void; | ||
| /** Default placeholder, matches Next.js. */ | ||
| bfcacheId?: string; | ||
| }; | ||
|
|
||
| /** | ||
| * Pages Router singleton surface — matches `NextRouter` from | ||
| * `packages/next/src/shared/lib/router/router.ts` (line 372). | ||
| * | ||
| * Typed loosely here because the helper is consumed by both the pages | ||
| * router shim (which exports the strict `NextRouter` type) and tests that | ||
| * only care about the runtime shape. The structural fields below are the | ||
| * subset that downstream callers (Next.js tests, third-party libs) read | ||
| * synchronously off `window.next.router`. | ||
| */ | ||
| export type PagesRouterPublicInstance = { | ||
| push: (...args: unknown[]) => unknown; | ||
| replace: (...args: unknown[]) => unknown; | ||
| back: () => void; | ||
| reload: () => void; | ||
| prefetch: (...args: unknown[]) => unknown; | ||
| beforePopState: (cb: (...args: unknown[]) => boolean) => void; | ||
| events: { | ||
| on: (event: string, handler: (...args: unknown[]) => void) => void; | ||
| off: (event: string, handler: (...args: unknown[]) => void) => void; | ||
| emit: (event: string, ...args: unknown[]) => void; | ||
| }; |
There was a problem hiding this comment.
do these types not already exist elsewhere?
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
Good approach — single installWindowNext() helper with merge semantics, one source of truth for the field shape, both routers calling in. Architecture matches AGENTS.md guidance (thin entries, real logic in typed modules). Tests are thorough.
Two things to address before merge:
1. CI failure: knip unused exported types (blocking)
The Check job fails because knip flags AppRouterPublicInstance and WindowNext as unused exported types. They're used inside the file's own declare global block and type definitions, but nothing outside window-next.ts imports them. Either drop the export keyword (they can stay as file-local types) or add a knip ignore comment if you want them available for future consumers.
2. Version field discrepancy (non-blocking)
The PR description says the version field reports the vinext package version (0.0.5), but neither call site passes version — router.ts:1042 and app-browser-entry.ts:1361 both omit it. The fallback is the literal string "vinext" (FALLBACK_VERSION). This is fine for unblocking the deploy suite (callers get a truthy string), but if the intent was to match Next.js's process.env.__NEXT_VERSION pattern, the actual package version should be injected via a build-time define or package.json import.
Minor notes
getWindowNext()is currently only consumed in tests. Fine as forward-looking API for the__pendingUrl/__internal_src_pagefollow-up, but knip may flag it too if it stays test-only.- The
as unknown as PagesRouterPublicInstancecast inrouter.ts:1042is well-commented and structurally correct at runtime. - Module-level
installedNext(line 137) introduces a subtle secondary tracking path alongsidewindow.next. Thewindow.next ?? installedNextfallback on line 158 means if someone deleteswindow.nextbut the module isn't re-imported,installedNextwould resurrect a stale reference. Not a practical issue given the usage pattern, just worth being aware of.
| * presence (`typeof router.hmrRefresh === "function"`) will skip the | ||
| * branch, matching what they would do on a production Next.js build. | ||
| */ | ||
| export type AppRouterPublicInstance = { |
There was a problem hiding this comment.
knip flags both AppRouterPublicInstance (here) and WindowNext (line 100) as unused exports — this is the Check CI failure. Neither type is imported outside this file. The declare global block on line 88 uses WindowNext but knip doesn't count intra-file usage as an external reference.
Simplest fix: drop the export keyword from both. They're only used within this file's own type definitions and ambient declarations. If you want them importable for future callers (e.g. the __pendingUrl follow-up), add a knip ignore directive instead.
| __internal_src_page?: string; | ||
| }; | ||
|
|
||
| const FALLBACK_VERSION = "vinext"; |
There was a problem hiding this comment.
The PR description says version reports 0.0.5 (the vinext package version), but neither router.ts nor app-browser-entry.ts passes a version field to installWindowNext(), so this fallback string "vinext" is what every user actually gets.
Not blocking — any truthy string unblocks the deploy suite — but if you want the real package version, consider either:
- A build-time
definereplacement (likeprocess.env.__VINEXT_VERSION) - Having the call sites pass it explicitly
Or update the PR description to match the actual behavior.
|
Review posted. Here's a summary: Review posted on PR #1201 with 1 summary comment and 2 inline comments. Key findings:
|
| return { version: fields.version ?? FALLBACK_VERSION, ...fields }; | ||
| } | ||
|
|
||
| const existing = window.next ?? installedNext; |
There was a problem hiding this comment.
would installedNext ever actually have a value when window.next doesn't?
…sion, remove module fallback - Drop `export` from `AppRouterPublicInstance` and `WindowNext` types (knip flagged them as unused; they're only used within window-next.ts). - Drop `getWindowNext()` and its export — tests read `window.next` directly, which is the same surface the runtime callers use. - Wire up `process.env.__NEXT_VERSION` via Vite `define` in index.ts, reading the vinext package version from packages/vinext/package.json at plugin load. This mirrors Next.js's own DefinePlugin substitution (packages/next/src/client/next.ts line 5) so library code that branches on `process.env.__NEXT_VERSION` sees a real value and `window.next.version` lands the package version instead of the `"vinext"` fallback. - Drop the module-level `installedNext` fallback. `installWindowNext` now reads and writes through `window.next` directly so deleting `window.next` from tests/userland fully resets state — no stale reference can be resurrected on the next call.
|
/bigbonk Addressed all review points:
|
There was a problem hiding this comment.
All four review points cleanly addressed:
-
exportdropped fromAppRouterPublicInstanceandWindowNext— confirmed atwindow-next.ts:48andwindow-next.ts:104.PagesRouterPublicInstancestays exported (line 68) and is consumed byrouter.ts:12. knip should be happy. -
getWindowNext()removed — gone from the module, tests now readwin.nextdirectly (e.g.shims.test.ts:678,:733,:738). Cleaner surface. -
Real version wired —
getVinextVersion()inindex.ts:356readspackage.json#versiononce (with cache),index.ts:1003injects it viadefine["process.env.__NEXT_VERSION"],window-next.ts:150reads it with"vinext"safety fallback, andglobal.d.ts:396declares theProcessEnvshape. Mirrors Next.js's own DefinePlugin substitution. The PR description now matches the code. -
installedNextmodule-level fallback removed —installWindowNextreads and writes throughwindow.nextdirectly (window-next.ts:170-185). Deletingwindow.nextcleanly resets state, no stale reference resurrection possible.
The declare global staying in window-next.ts (not moved to global.d.ts) is the right call — global.d.ts:23-25 and :147-148 explain the co-location rationale, and it avoids a circular dependency (the WindowNext type would need to be imported into global.d.ts from the module that owns it).
Ship it.
|
Review posted with approval on PR #1201. All four review points are cleanly addressed:
The |
1 participant
Summary
Adds the
window.nextglobal that Next.js publishes on the client from both the Pages Router bootstrap and the App Router bootstrap. vinext did not install this global, which broke any caller that readswindow.next.routerdirectly — including the Next.js deploy test suite, Pages Router tests in userland, and several third-party libraries.Deploy suite impact
Before this PR the Next.js deploy suite reported:
window.next.*TypeError: Cannot read properties of undefined (reading 'router').__internal_src_pageand__pendingUrlranked next.After this PR those callers find the expected shape on
window.nextand the errors clear.What was added
A typed
installWindowNext()helper lives inpackages/vinext/src/client/window-next.ts. Both router bootstraps invoke it, so the field shape stays in one place per AGENTS.md's guidance to keep generated entries thin and put real logic inclient//server/typed modules.window.next.*fieldversionpackages/next/src/client/next.tsline 13 (Pages);packages/next/src/client/app-bootstrap.tsline 13 (App). Reports the vinext package version, injected via Vitedefineforprocess.env.__NEXT_VERSION(mirroring Next.js's own DefinePlugin).appDirpackages/next/src/client/app-bootstrap.tsline 15 (appDir: true)router(App Router)packages/next/src/client/components/app-router-instance.tsline 510 (window.next.router = publicAppRouterInstance). Wired to vinext'sappRouterInstance(the same singletonuseRouter()returns).router(Pages Router)packages/next/src/client/next.tsline 16-18 (live-binded getter). Wired to vinext's PagesRoutersingleton fromnext/router.router.eventspackages/next/src/shared/lib/router/router.tsline 372 (NextRoutertype). Tests liketest/development/pages-dir/client-navigation/index.test.ts:457rely onwindow.next.router.events.on(...).__pendingUrlpackages/next/src/client/components/app-router-instance.tsline 296; read bypackages/next/src/client/components/nav-failure-handler.ts__internal_src_pagepackages/next/src/client/components/app-router.tsxline 204Version wiring
The Vite plugin reads
packages/vinext/package.json#versionat plugin load (getVinextVersion()inindex.ts) and exposes it viadefineasprocess.env.__NEXT_VERSION. Thewindow-next.tsmodule reads that value and uses it as the default forinstallWindowNext({ version }). Mirrors how Next.js substitutes the same constant via its webpackDefinePlugin. Library code that readsprocess.env.__NEXT_VERSIONdirectly now also sees a real value instead ofundefined.The standalone
"vinext"literal remains as a safety fallback for any code path that importsclient/window-next.tswithout going through the Vite plugin (unit tests that don't exercise the build).Fields intentionally omitted
emitter(Pages,packages/next/src/client/next.tsline 19) — this is the page-loadmitt()emitter, distinct fromrouter.events. vinext does not currently fire those events anywhere, so exposing it would be a fake surface. Left as a follow-up if it shows up in the deploy suite.router.hmrRefreshandrouter.experimental_gesturePush(App Router) — vinext does not implement either, so they remain undefined. Library callers that probetypeof router.hmrRefresh === "function"skip the branch, matching production Next.js behavior.turbopackflag frompackages/next/src/client/app-next-turbopack.ts— vinext doesn't use Turbopack, so this flag is not meaningful.Open questions / follow-ups
__pendingUrland__internal_src_pageare populated only when the App Router's per-navigation code paths get wired up to write them. Initial install leaves bothundefined(matching Next.js's initial-load state), but full parity requires hooking the writes insideserver/app-browser-entry.ts's navigation controller. If the deploy suite still complains about these specific fields after this PR, that follow-up should land next.Tests
Added 6 focused tests under
tests/shims.test.tsin a newwindow.next debug globaldescribe block:installWindowNextpopulatesversion,router, andappDirrouterwhen both routers are present)NextRoutersurface at module loadwindow.next.router.eventsforwards Pages Router eventsappRouterInstancenamed export has the App Router public surfaceinstallWindowNextis a no-op whentypeof window === "undefined"Full
tests/shims.test.ts(866 tests) andtests/link.test.ts, plustests/app-browser-entry.test.ts,tests/app-router.test.ts, andtests/pages-router.test.ts(605 tests) all pass.tests/build-optimization.test.ts(73 tests) also passes.vp run knipis clean.vp checkis clean for code in this PR. There is one pre-existing lint error intests/e2e/fixtures.ts(also present onorigin/main) that is unrelated.