fix(shims): throw Pages Router parity error from unstable_retry (#1448)#1670
Conversation
Closes the Pages Router parity gap noted as a follow-up in #1206. When `unstable_catchError`'s boundary is rendered under the Pages Router, `unstable_retry()` now throws Next.js's verbatim error message ("`unstable_retry()` can only be used in the App Router. Use `reset()` in the Pages Router.") instead of calling App Router's `refresh()` (which is a no-op there and leaves the user stuck on the error). Detection mirrors Next.js's own implementation in `packages/next/src/client/components/catch-error.tsx`: the inner class component reads `RouterContext` via `static contextType`. The wrapper stays a pure JSX factory so existing introspection-style unit tests that invoke it as a bare function continue to work. Regression test ported from `test/e2e/app-dir/catch-error/catch-error.test.ts` — "should throw when unstable_retry is called on Pages Router".
|
/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.
Review: fix(shims): throw Pages Router parity error from unstable_retry
Looks good. The implementation is correct and the tests cover the important branches. A few observations:
Correctness
The context detection logic is sound. The approach is inverted relative to Next.js (vinext reads RouterContext and checks !== null for Pages Router, while Next.js reads AppRouterContext and checks === null for "not App Router"), but the end result is equivalent:
RouterContextdefaults tonullviacreateContext<NextRouter | null>(null)- Pages Router pages are always wrapped in
RouterContext.ProviderbywrapWithRouterContext()(shims/router.ts:1613) - Under App Router, no
RouterContext.Providerexists →this.contextstaysnull - So
this.context !== null↔ "we're under Pages Router" ✓
This approach is actually slightly cleaner for vinext because AppRouterContext can itself be null at the module level (if React.createContext is unavailable), which would add an extra edge case.
Test fixes for existing tests
The seeding of (instance).context = null in the two existing App Router tests is correct and necessary. Without it, this.context would be undefined (manual new skips React's context wiring), and undefined !== null is true, which would incorrectly trigger the Pages Router error path. Good catch.
New test
The Pages Router regression test seeds this.context = { route: "/some-page" } and asserts the verbatim error message. This matches the expected production behavior where React fills this.context from RouterContext.Provider.
Minor note
The void React on line 1406 of the test exists solely to suppress the "unused import" lint. It's a pattern used in sibling tests — fine to keep for consistency.
LGTM.
|
Review posted. The PR is approved — the implementation correctly detects Pages Router via |
1 participant
Summary
Closes the Pages Router parity gap explicitly tracked as a follow-up in #1206, addressing #1448.
When
unstable_catchError's boundary is rendered under the Pages Router, callingunstable_retry()previously invokedappRouterInstance.refresh()— a no-op under Pages Router that left the error visible untilreset()was called. Next.js documentsunstable_retryas App Router only and throws a specific error message from the boundary itself. We now match that behavior verbatim.RouterContextviastatic contextType(mirroring Next.js'sstatic contextType = AppRouterContextpattern inclient/components/catch-error.tsx).this.context !== null(Pages Router page),unstable_retry()throws`unstable_retry()` can only be used in the App Router. Use `reset()` in the Pages Router..Regression test ported from Next.js' own suite:
test/e2e/app-dir/catch-error/catch-error.test.ts— "should throw when unstable_retry is called on Pages Router".Files
packages/vinext/src/shims/error.tsx— wireRouterContextascontextType, branch inunstable_retry, refresh the doc comment.tests/shims.test.ts— new Pages Router branch test; seedthis.context = nullon the two existing App Router tests (manual class instantiation skips React's context wiring).Test plan
pnpm test tests/shims.test.ts -t "unstable_catchError"— 10/10 pass.pnpm test tests/shims.test.ts tests/error-boundary.test.ts— 1046/1046 pass.pnpm run check— clean (format + lint + type).Out of scope (documented follow-ups remaining from #1206)
unstable_retry(requires Suspense-boundary re-fetch wiring).handleHardNavError,handleISRError.