fix(shims): add withRouter HOC to next/router#1203
Conversation
Add the `withRouter` higher-order component export to `packages/vinext/src/shims/router.ts`, ported from `packages/next/src/client/with-router.tsx` in Next.js. `withRouter` is used primarily by class components (which cannot call hooks) to receive the Pages Router `router` instance as a prop. Real Next.js apps and the Next.js deploy test suite import it from `next/router`; without this export, bundlers fail with `[MISSING_EXPORT] "withRouter" is not exported by ".../shims/router.js"`, which is currently the cause of 9 build failures in the deploy suite. The implementation mirrors Next.js's HOC: it calls `useRouter()` and forwards the router as a `router` prop, forwards `getInitialProps` and `origGetInitialProps` from the composed component, and sets a `withRouter(<Name>)` displayName in development. Also exports the `WithRouterProps` and `ExcludeRouterProps` types (commonly imported by typed Pages Router apps) and adds matching `declare module "next/router"` entries.
|
/bigbonk review |
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean port — the runtime behavior matches Next.js and the tests cover the key surface. One correctness issue with prop spreading order that could cause a subtle behavioral divergence, plus a few minor nits.
CI note: The failing Vitest check (tests/link-navigation.test.ts > clicking an RSC Link starts app-router navigation inside a React transition) is unrelated to this PR — it's a pre-existing flake in the link navigation test.
| return createElement(ComposedComponent, { | ||
| ...(props as unknown as P), | ||
| router, | ||
| }); |
There was a problem hiding this comment.
Bug: Prop spreading order differs from Next.js — user router prop silently wins over the HOC-injected one.
In Next.js's with-router.tsx, the order is:
<ComposedComponent router={useRouter()} {...props} />This means if a caller accidentally passes a router prop to the wrapped component, the caller's value overrides the HOC-injected one (props spread last). This is the documented Next.js behavior — the last-spread wins.
But here the order is reversed:
createElement(ComposedComponent, {
...(props as unknown as P),
router, // ← always wins
});The HOC-injected router always wins, which means a user-passed router prop is silently swallowed.
In practice this rarely matters (nobody should be passing router to a withRouter-wrapped component), but for Next.js behavioral parity and to match the source you're porting from, the order should be:
createElement(ComposedComponent, {
router,
...(props as unknown as P),
});This also matches the ExcludeRouterProps<P> type constraint — the wrapper's props type already excludes router, so in well-typed code the caller can't pass it. But in JavaScript (no types) or with as any casts, the spreading order is the behavioral contract.
| getInitialProps?: unknown; | ||
| origGetInitialProps?: unknown; | ||
| }; | ||
| (WithRouterWrapper as unknown as { getInitialProps?: unknown }).getInitialProps = |
There was a problem hiding this comment.
Nit: In Next.js, getInitialProps is assigned directly without the intermediate cast:
WithRouterWrapper.getInitialProps = ComposedComponent.getInitialPropsBecause NextComponentType has getInitialProps on its type, there's no cast needed. Here, since ComposedComponent is typed as ComponentType<P> (which lacks getInitialProps), the casts are necessary. That's fine — just noting this is a consequence of the type narrowing documented in the PR description. The runtime behavior is correct.
One thing to consider: if ComposedComponent.getInitialProps is undefined (which it will be for most function components), you're assigning undefined to WithRouterWrapper.getInitialProps. Next.js does the same, so this is correct — but it means the property exists (as undefined) on the wrapper. That's fine for _app checks that use in operator or hasOwnProperty, since they'd find the key. Just flagging for visibility — no change needed.
| import { ComponentType } from "react"; | ||
| export function useRouter(): any; | ||
| export function setSSRContext(ctx: any): void; | ||
| export type WithRouterProps = { router: any }; |
There was a problem hiding this comment.
Nit: This is typed as { router: any } while the runtime export uses { router: NextRouter }. The PR description explains this is intentional (keeping the d.ts consistent with the existing any pattern for useRouter). That's a reasonable consistency choice — just noting it for anyone reading later who wonders why the types diverge.
| // --------------------------------------------------------------------------- | ||
| describe("next/router withRouter HOC", () => { | ||
| it("exports withRouter as a named function", async () => { | ||
| const previousWindow = (globalThis as any).window; |
There was a problem hiding this comment.
Nit: Repeated window mock setup across all 4 tests.
Each test in this describe block creates the same win object and does the same globalThis.window swap + vi.resetModules() + finally restore. Consider extracting this into a beforeEach/afterEach pair within the describe block:
describe("next/router withRouter HOC", () => {
let previousWindow: any;
beforeEach(() => {
previousWindow = (globalThis as any).window;
(globalThis as any).window = {
location: { pathname: "/", search: "", hash: "", href: "http://localhost/" },
history: { state: null, pushState() {}, replaceState() {} },
addEventListener() {},
};
vi.resetModules();
});
afterEach(() => {
(globalThis as any).window = previousWindow;
vi.resetModules();
});
// ...tests without the boilerplate
});This would cut ~60 lines of boilerplate. Not a blocker — the existing pattern is fine and matches the style in the existing window.next tests above.
|
Review posted successfully. Here's a summary of what I found: Review Summary for PR #1203Overall: Clean port of Key finding — prop spreading order bugThe main issue is a prop spreading order difference from Next.js at <ComposedComponent router={useRouter()} {...props} />Props spread after Other notes:
|
Address bonk review on PR #1203: 1. (Correctness) Match the Next.js prop-spread order in `withRouter`. Next.js's with-router.tsx renders <ComposedComponent router={useRouter()} {...props} /> so the injected `router` is placed FIRST and `{...props}` is spread after — a user-passed `router` prop wins by last-spread. The previous implementation spread `props` first and then re-assigned `router`, which silently swallowed any user-passed `router` prop. Now matches Next.js exactly. Added a regression test that passes a sentinel `router` prop and asserts the user-passed value reaches the composed component. 2. (Cleanup) Extracted the repeated globalThis.window mock and vi.resetModules() teardown in the withRouter describe block into beforeEach/afterEach. Cuts ~60 lines from tests/shims.test.ts.
|
/bigbonk review |
There was a problem hiding this comment.
Both review items from the first pass are addressed in the second commit:
-
Prop spreading order — fixed.
{ router, ...(props as Record<string, unknown>) }now matches Next.js's<ComposedComponent router={useRouter()} {...props} />exactly (HOC router first, user props spread after). The new regression test attests/shims.test.ts:907validates this with a sentinel object. -
Test boilerplate — cleaned up. The window mock is now in
beforeEach/afterEachwithin the describe block.
CI: Check, Vitest (unit), all E2E projects that have completed, all deploys, and smoke tests are green. A few integration shards are still pending but unrelated to this change.
Implementation is clean and correct. LGTM.
|
Review posted. Both items from the previous review — the prop spreading order bug and the test boilerplate — are addressed in the second commit. CI is green across all completed checks. Approved. |
1 participant
Summary
withRouterhigher-order component topackages/vinext/src/shims/router.ts, ported frompackages/next/src/client/with-router.tsxin Next.js.WithRouterPropsandExcludeRouterPropstypes (commonly imported by typed Pages Router apps) and add matchingdeclare module "next/router"entries innext-shims.d.ts.Why
The Next.js deploy test suite currently has 9 build failures of the form:
withRouteris primarily used by class components (which cannot call hooks) to receive the Pages Routerrouterinstance as a prop. Real-world Next.js apps and the e2e fixtures (e.g.test/e2e/with-router/pages/a.js) import it directly fromnext/router. Without this export, those bundles fail to build.Implementation
The implementation mirrors Next.js's HOC behaviour:
useRouter()and forwards the result as arouterprop.getInitialPropsandorigGetInitialPropsfrom the composed component (parity with_appPages Router behaviour for class components that definegetInitialProps).withRouter(<Name>)displayNameoutsideproduction.Source:
packages/next/src/client/with-router.tsxType signature differences vs Next.js
Next.js types the composed component as
NextComponentType<C, any, P>. vinext does not currently exposeNextComponentTypefrom this shim, so the port usesComponentType<P>instead. The runtime shape is identical; the only difference is that vinext's HOC cannot recover the legacyNextComponentType.contexttype. No existing call sites in the Next.js test fixtures depend on that — they all use plain class components that readthis.props.router.Tests
Added a
next/router withRouter HOCdescribe block intests/shims.test.ts(4 tests):withRouteris exported as a function.withRouter(...)displayNamein dev.routeras a prop with the expectedNextRoutersurface (push,replace,back,reload,prefetch,events).getInitialPropsandorigGetInitialPropsare forwarded from the composed component.Local results:
vp test run tests/shims.test.ts— 870 passed (4 new).vp check— pass (format, lint, type checks).Uncertainty / follow-ups
ComponentType<P>instead ofNextComponentType<C, any, P>) is a deliberate narrowing because vinext does not exposeNextComponentType. If a user's TypeScript code depends on the richer Next.js signature, they may see a type-only regression. The runtime behaviour is identical and matches Next.js exactly.WithRouterProps.routerinnext-shims.d.tsis typed asanyrather than the fullNextRouterinterface; the ambient declaration file already usesanyforuseRouter's return value, so this keeps the d.ts file consistent. The actual implementation (inrouter.ts) uses the fullNextRoutertype.