fix(shims): add unstable_catchError, unstable_rethrow, unstable_isUnrecognizedActionError#1206
Conversation
…ecognizedActionError
Unblocks 18 build failures in the Next.js deploy suite:
- `unstable_catchError` (12 failures) — added to `shims/error.tsx`. App
Router error-boundary HOC ported from Next.js's
`client/components/catch-error.tsx`. Implements the same
`getDerivedStateFromError` predicate that re-throws Next.js navigation
signals (redirect, notFound, …) and renders the user fallback with an
`ErrorInfo` value otherwise. `unstable_retry` throws a clear "not yet
implemented" error (tracked as follow-up).
- `unstable_rethrow` (3 failures) — added to `shims/navigation.ts` and
re-exported from `shims/navigation.react-server.ts`. Ported from
Next.js's `client/components/unstable-rethrow.{ts,server,browser}.ts`.
Specialized to the internal-error categories vinext actually emits
(redirect + HTTP access fallback); recurses through `error.cause` for
wrapped errors.
- `unstable_isUnrecognizedActionError` (3 failures) — added to
`shims/navigation.ts` along with the `UnrecognizedActionError` class.
Ported 1:1 from `client/components/unrecognized-action-error.ts`. The
react-server condition exports a throwing stub matching Next.js's
`navigation.react-server.ts`.
Also adds `isRedirectError` and `isNextRouterError` helpers to
`navigation.ts` (used internally by `unstable_rethrow`) and updates
`next-shims.d.ts` so the new exports type-check for users.
|
/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.
Good work overall — the new exports are well-documented, tested, and the PR description is thorough. The implementation correctly captures the core behavior of the Next.js APIs it ports. A few observations below.
isRedirectError divergence from Next.js
The biggest intentional divergence worth calling out explicitly: vinext's isRedirectError does a permissive prefix check (digest.startsWith("NEXT_REDIRECT;")), while Next.js's version does full 4-segment validation — it parses the digest, verifies the type is "push" | "replace", checks the destination is a non-empty string, and validates the status code is one of 303/307/308.
This is fine as a practical choice (vinext's own digest format is 3-part or 4-part, not Next.js's 5-part), but it means isRedirectError will return true for malformed digests like "NEXT_REDIRECT;garbage". Since this is now a public API (exported from next/navigation), userland code that relies on the return value to trust the digest structure could be surprised. Worth a one-line doc comment noting the permissive matching.
This also means isNextRouterError inherits the same permissiveness, which flows into unstable_rethrow and getDerivedStateFromError in the catch-error boundary — those will rethrow anything with a NEXT_REDIRECT; prefix, even if the digest is invalid.
Duplicate isRedirectError in error-boundary.tsx
error-boundary.tsx:74 has a private isRedirectError with essentially the same logic. Now that a public version lives in navigation.ts, consider consolidating in a follow-up — the error-boundary could import from ./navigation.js (same pattern as error.tsx already does for isNextRouterError). Not blocking, but worth tracking.
unstable_rethrow cause recursion throws the inner error, not the outer
The recursion in unstable_rethrow is faithful to Next.js, but the behavior is subtle: when a user wraps a redirect error in new Error("wrapped", { cause: redirectError }), the function rethrows the inner redirect error, not the wrapper. The test at line 714-715 correctly validates this. Just flagging that this is intentional and matches Next.js.
unstable_isUnrecognizedActionError redundant checks
The predicate !!(error && typeof error === 'object' && error instanceof UnrecognizedActionError) has redundant guards (error && and typeof error === 'object' are both implied by instanceof succeeding). This matches Next.js's implementation verbatim, so keeping it for 1:1 parity is fine.
_CatchError.displayName vs wrapper displayName
The inner class has displayName = "unstable_catchError(Next.CatchError)" (matching Next.js), while the wrapper function gets displayName = "unstable_catchError(MyFallback)" with the user's fallback name. This is the correct pattern — DevTools will show the wrapper's displayName. Good.
react-server unstable_isUnrecognizedActionError return type
The stub returns boolean (not never) despite always throwing — this matches Next.js exactly and is correct for type-level compatibility with the client signature.
Test approach for unstable_catchError
The manual class instantiation approach in the test (lines 825-873) is pragmatic given React 19's SSR behavior. The comment explaining why is helpful. One thing: the test calls new InnerCatchError(props) directly, bypassing React's component lifecycle — this means this.props isn't set by React's machinery. It works because the test manually sets instance.state and calls instance.render(), but this.props access in render() relies on the constructor arg being stored by React.Component's base constructor. This is fine but fragile — if React ever changes its base class constructor behavior, these tests break. Not actionable, just noting.
No blocking issues. The implementation is solid, well-scoped, and the follow-up items (consolidating duplicate isRedirectError, wiring unstable_retry, extending unstable_rethrow categories) are clearly documented.
|
Review posted on PR #1206. Here's a summary of the findings: No blocking issues. The implementation is solid and well-scoped. Key observations:
|
…dary copy Addresses two /bigbonk review observations on #1206: 1. **Document the permissive prefix match.** The public `isRedirectError` in `shims/navigation.ts` does `digest.startsWith("NEXT_REDIRECT;")` while Next.js's `client/components/redirect-error.ts` does full 4-segment validation (type ∈ {push, replace}, non-empty destination, status ∈ {303, 307, 308}). The divergence is intentional — vinext emits 3-part and 4-part digests whereas Next.js's validator targets its 5-part canary digests — but now that the predicate is reachable from the public `next/navigation` surface via `unstable_rethrow`, it gets a prominent JSDoc block explaining the difference, the consequence (malformed digests return true here, false in Next.js), and a link to the Next.js source. 2. **Consolidate duplicate `isRedirectError`.** `shims/error-boundary.tsx` had its own private copy with the same prefix-match logic. Replaced the duplicate with an import from `./navigation.js`, matching the pattern already used by `shims/error.tsx` (`import { isNextRouterError } from "./navigation.js"`). The `RedirectBoundary` call site casts to the file-local `RedirectError` type to preserve access to the optional `handled` field — this is safe by construction (every error matching the prefix predicate is produced by vinext's `redirect()` / `permanentRedirect()` helpers, which yield `Error` instances). No behavior change. `vp check` + `vp test run tests/shims.test.ts tests/error-boundary.test.ts` (916 tests) pass.
|
/bigbonk review |
There was a problem hiding this comment.
Solid PR — well-scoped, well-documented, and the implementation correctly captures Next.js behavior for all three unstable_* APIs. Tests are thorough. A few observations below, none blocking.
error-boundary.tsx consolidation is clean
The old private isRedirectError + getErrorDigest import is cleanly replaced by the public isRedirectError from ./navigation.js. The getErrorDigest import removal is correct since it was only used by the now-removed private function (the remaining isNavigationSignalError import from ../utils/navigation-signal.js is unaffected). The cast at line 149 from the narrowed Error & { digest: string } to the local RedirectError type (which adds handled?) is safe and well-documented.
Minor style inconsistency between isRedirectError and isHTTPAccessFallbackError
The new isRedirectError uses typeof (error as ...).digest !== "string" as a strict guard, while the existing isHTTPAccessFallbackError at line 1480 uses String() coercion. The strict version is actually better — String(undefined) yields "undefined" which could theoretically pass a prefix check, while typeof correctly rejects non-string digests. This is a pre-existing pattern, not introduced by this PR, so non-blocking. Could be a good cleanup in a follow-up to align both predicates.
unstable_retry throwing stub is the right call
Throwing a clear error is strictly better than a silent no-op. When eventually wired up, the boundary will need access to the App Router instance (likely via context), which aligns with the follow-up tracking in the PR description and code comments.
unstable_rethrow cause recursion — confirmed correct
The subtle behavior of rethrowing the inner error (the cause) rather than the wrapper is faithful to Next.js and correctly validated by the test at line 714-715.
| * When wrapping an API that uses errors for control flow (redirect, notFound, | ||
| * etc.), call this inside `catch` blocks before doing your own error handling. | ||
| * If the error is a Next.js internal error, it's rethrown; otherwise this is a | ||
| * no-op (apart from recursing through `error.cause`). |
There was a problem hiding this comment.
Nit: the defensive guard chain here (!error || typeof error !== "object" || !("digest" in error) || typeof ... !== "string") is stricter than the existing isHTTPAccessFallbackError at line 1480, which uses String() coercion. The stricter version is better — String(undefined) yields "undefined" which could pass a prefix check. Consider aligning isHTTPAccessFallbackError to the same pattern in a follow-up.
| // Match Next.js's DevTools label so userland tooling/snapshots align. | ||
| // https://github.com/vercel/next.js/blob/canary/packages/next/src/client/components/catch-error.tsx | ||
| static displayName = "unstable_catchError(Next.CatchError)"; | ||
|
|
There was a problem hiding this comment.
Good decision to throw rather than no-op. When this gets wired up, the boundary will need the App Router instance threaded through (context or ref). The comment tracking this is clear.
After consolidating shims/error-boundary.tsx onto the public isRedirectError from shims/navigation.ts, the only remaining caller of getErrorDigest is inside utils/navigation-signal.ts itself. Knip flags the export as unused, which fails the Check job in CI. Convert getErrorDigest to a file-local function. No behavior change.
|
Review posted and approved on PR #1206. Here's a summary: Verdict: APPROVE — no blocking issues. Key observations:
|
Parity audit findings on PR #1206 (post /bigbonk APPROVED) revealed two gaps from a thorough re-read of Next.js's error-rethrow surface and `unstable_catchError` test fixtures. ## 1. `unstable_rethrow` — added missing error categories Next.js's server-side `unstable_rethrow` rethrows seven error categories (client-side rethrows two). vinext was only handling #1 (`isNextRouterError`). The canonical user-facing fixture (`test/e2e/app-dir/app-static/lib/fetch-retry.js`) breaks if any of the other categories are not propagated. Added the two categories vinext can plausibly encounter: - `BailoutToCSRError` + `isBailoutToCSRError` — thrown by `next/dynamic` with `ssr: false`. Lives in shared (non-server) code so third-party libraries and accidentally-bundled Next.js internals can produce it. Ported 1:1 from https://github.com/vercel/next.js/blob/canary/packages/next/src/shared/lib/lazy-dynamic/bailout-to-csr.ts - `DynamicServerError` + `isDynamicServerError` — thrown by Next.js's `cookies()`/`headers()` when called in a static render context. vinext doesn't construct it itself but exposes the class so user code, action wrappers, and accidental Next.js-internal bundles can interoperate. Ported 1:1 from https://github.com/vercel/next.js/blob/canary/packages/next/src/client/components/hooks-server-context.ts The remaining four server-only categories (`isDynamicPostpone`, `isPostpone`, `isHangingPromiseRejectionError`, `isPrerenderInterruptedError`) are tied to Next.js's PPR / prerender controller machinery that vinext does not implement. User code cannot construct them in normal use; they're deferred as a follow-up (documented inline). A test pins this intentional gap. ## 2. `unstable_catchError.unstable_retry` — made functional The previous implementation threw a generic "not yet implemented" error in both server and client contexts. Now matches Next.js's App Router branch: on the client, calls `appRouterInstance.refresh()` inside `React.startTransition` and resets the boundary. On the server, throws a clear "client-only" error (refresh is meaningless during SSR setup). Pages Router's exact error message (`unstable_retry() can only be used in the App Router. Use reset() in the Pages Router.`) is not yet dispatched because vinext's boundary doesn't read `PagesRouterContext`; documented in the JSDoc as a parity follow-up. ## Test coverage Added 8 new test cases covering: - `BailoutToCSRError` + predicate (canonical digest, foreign-object detection, negative cases) - `DynamicServerError` + predicate (same shape) - `unstable_rethrow` propagates both new categories (re-throw identity) - `unstable_rethrow` does NOT match the four uncovered server-only categories (pins the intentional gap) - `unstable_catchError.getDerivedStateFromError` accepts `null` / `undefined` thrown values (ported from test/e2e/app-dir/catch-error/throw-null and throw-undefined fixtures) - `unstable_retry` throws "client-only" on the server - `unstable_retry` on the client calls `appRouterInstance.refresh()` + resets via setState Also added `isRedirectError` / `isNextRouterError` / new predicates JSDoc note that these are vinext-only extensions (Next.js does NOT export them from `next/navigation` — confirmed by reading `packages/next/src/client/components/navigation.ts` end-to-end). `vp test run tests/shims.test.ts tests/error-boundary.test.ts tests/app-router.test.ts tests/app-rsc-errors.test.ts` — 1274 tests pass. `vp check` clean.
|
/bigbonk review |
There was a problem hiding this comment.
LGTM — well-executed PR with thorough documentation and test coverage. The implementation is correct and the consolidation of isRedirectError into the canonical module is a clean improvement.
Verified
- Shim wiring is complete. All new exports from
navigation.tsanderror.tsxare automatically exposed atnext/navigationandnext/errorimport paths — no explicit export list needs updating. The react-server variants are correctly configured. error-boundary.tsxconsolidation is clean. The old privateisRedirectError+getErrorDigestimport is replaced by the publicisRedirectErrorfrom./navigation.js. The cast from the narrowedError & { digest: string }to the localRedirectError(which addshandled?) is safe and well-documented.unstable_rethrowcause recursion is correct. Rethrows the inner cause (the framework signal), not the wrapper. Test at line 812 pins this.unstable_catchErrorHOC pattern matches Next.js. The_CatchErrorclass component correctly rethrows navigation signals fromgetDerivedStateFromError, theunstable_retryimplementation on the client mirrors Next.js's App Router branch (refresh()+reset()instartTransition), and the server-side throw is the right call.- Test coverage is thorough. 22 new test cases covering happy paths, edge cases (null/undefined thrown values), negative cases, cause recursion, intentional gap pinning for the four uncovered server-only categories, and client-side
unstable_retrybehavior. - Type declarations in
next-shims.d.tsare complete for all new exports.
Non-blocking observations
-
isHTTPAccessFallbackErrorguard style inconsistency — pre-existing, not introduced here. The newisRedirectErroruses the strictertypeof digest !== "string"guard, while the existingisHTTPAccessFallbackErrorat line 1480 usesString()coercion. The strict version is better. Aligning the older predicate would be a low-risk follow-up. -
isDynamicServerErrorhas a redundant comparison — the predicate at line 1795 checkstypeof digest === "string" && digest === _DYNAMIC_SERVER_USAGE_DIGEST. Thetypeofcheck is redundant since===with a string literal already requires the operand to be a string. Not wrong, just unnecessary — and consistent with the defensive style used elsewhere.
|
Review posted and approved on PR #1206. The PR is solid — correct implementation, thorough tests (22 new cases), complete type declarations, and clean consolidation of the duplicate |
Bonk-review observation on #1206: the predicate was checking `typeof digest === "string" && digest === _DYNAMIC_SERVER_USAGE_DIGEST`, where the `===` against a string literal already requires the operand to be a string. Drop the redundant guard for consistency with `isBailoutToCSRError` above. Non-blocking cleanup; no behavior change.
|
/bigbonk re-review |
There was a problem hiding this comment.
Re-reviewed after the latest push. The PR remains clean — no blocking issues. Approving.
Verified
-
error-boundary.tsxconsolidation — The old privateisRedirectError+getErrorDigestimport is replaced by the publicisRedirectErrorfrom./navigation.js. The cast at line 149 fromError & { digest: string }to the localRedirectError(which addshandled?) is safe and documented. ThegetErrorDigestexport removal innavigation-signal.tsis correct — grep confirms it's now only used internally byisNavigationSignalErrorwithin the same file. -
Predicate implementations —
isRedirectError,isNextRouterError,isBailoutToCSRError,isDynamicServerError, andunstable_isUnrecognizedActionErrorall have correct guard chains and match their Next.js counterparts (or document the intentional divergence, inisRedirectError's case). -
unstable_rethrowcause recursion — Correctly rethrows the inner cause (the framework signal), not the wrapper. This subtle behavior matches Next.js and is properly pinned by the test at line 812. -
unstable_catchErrorHOC — Clean class-component error boundary.getDerivedStateFromErrorcorrectly rethrows navigation signals viaisNextRouterError, preventing the boundary from swallowing redirects/notFound. Theunstable_retryimplementation correctly branches ontypeof window === "undefined"— throwing on the server and doingstartTransition(refresh + reset)on the client. -
react-server re-exports — All environment-agnostic predicates and error classes are re-exported from
navigation.react-server.ts.unstable_isUnrecognizedActionErrorcorrectly has a standalone throwing stub (not re-exported), matching Next.js's pattern where it's genuinely client-only. -
Type declarations —
next-shims.d.tscovers all new exports for bothnext/navigationandnext/error. -
Tests — 22 new test cases are thorough. The intentional-gap pinning test for the four uncovered server-only categories is particularly valuable for future maintainers.
Non-blocking observations (tracked by prior reviews)
-
isHTTPAccessFallbackErrorusesString()coercion (line 1482) while the newisRedirectErrorusestypeof digest !== "string"strict guard (line 1666). The strict version is better —String(undefined)yields"undefined"which could theoretically pass a prefix check. Pre-existing pattern, not introduced here. -
UnrecognizedActionErrorinnext-shims.d.ts(line 179) declares the class asextends Error {}without thenameproperty. The implementation setsthis.name = "UnrecognizedActionError"in the constructor — the declaration could includereadonly name: "UnrecognizedActionError"for completeness, but since it's inherited fromErrorthis is cosmetic.
|
Review posted and approved on PR #1206. Summary: Verdict: APPROVE — no blocking issues found on re-review. The implementation is correct across all seven changed files. Key verifications:
Two non-blocking style observations noted (pre-existing |
1 participant
Summary
Adds missing
unstable_*exports related to error handling, unblocking 18 build failures in the Next.js deploy suite. After a thorough parity audit against Next.js's full error-rethrow surface, the PR also coversBailoutToCSRErrorandDynamicServerErrorand makesunstable_catchError.unstable_retryfunctional on the client.Exports
unstable_catchErrorpackages/vinext/src/shims/error.tsxclient/components/catch-error.tsx+api/error.tsisNextRouterError.unstable_retrymatches Next.js's App Router branch: callsappRouterInstance.refresh()insideReact.startTransitionthen resets the boundary. On the server, throws a clear "client-only" error.unstable_rethrowpackages/vinext/src/shims/navigation.ts(+ re-exported fromnavigation.react-server.ts)client/components/unstable-rethrow.{ts,server,browser}.tsisNextRouterError,isBailoutToCSRError) plus one of the five server-build extras (isDynamicServerError). The remaining four server-only categories (PPR / prerender controller) are deferred — vinext can't emit them. Recurses througherror.cause.isRedirectErrorpackages/vinext/src/shims/navigation.tsclient/components/redirect-error.tsisRedirectErrorfromnext/navigation. Permissive prefix match (vs. Next.js's strict 4-segment validation) because vinext emits 3- and 4-part digests where Next.js emits 5-part. Divergence documented in JSDoc.isNextRouterErrorpackages/vinext/src/shims/navigation.tsclient/components/is-next-router-error.tsisRedirectError. CombinesisRedirectError+isHTTPAccessFallbackError.BailoutToCSRError+isBailoutToCSRErrorpackages/vinext/src/shims/navigation.tsshared/lib/lazy-dynamic/bailout-to-csr.tsBAILOUT_TO_CLIENT_SIDE_RENDERING) is the stable detection contract.DynamicServerError+isDynamicServerErrorpackages/vinext/src/shims/navigation.tsclient/components/hooks-server-context.tsBailoutToCSRError.UnrecognizedActionErrorpackages/vinext/src/shims/navigation.tsclient/components/unrecognized-action-error.tsunstable_isUnrecognizedActionErrorpackages/vinext/src/shims/navigation.ts(+ throwing stub innavigation.react-server.ts)client/components/unrecognized-action-error.ts+client/components/navigation.react-server.ts'client only'throw.unstable_rethrowcategory coverageNext.js's server-side build handles seven error categories; the browser build handles two. vinext now covers three, all in a single environment-agnostic implementation:
isNextRouterError(redirect + HTTP fallback)is-next-router-error.tsisBailoutToCSRError(next/dynamicssr: false)bailout-to-csr.tsisDynamicServerError(dynamic API in static render)hooks-server-context.tsisDynamicPostpone(PPR message check)dynamic-rendering.ts:408isPostpone(React.unstable_postpone)is-postpone.tsisHangingPromiseRejectionError(prerender abort)dynamic-rendering-utils.tsisPrerenderInterruptedError(prerender interrupt)dynamic-rendering.ts:448The four uncovered categories are tied to Next.js's PPR / prerender-controller machinery that vinext does not implement; user code cannot construct them in normal use. A dedicated test pins this intentional omission so the gap is visible to future maintainers.
Files touched
packages/vinext/src/shims/error.tsxpackages/vinext/src/shims/navigation.tspackages/vinext/src/shims/navigation.react-server.tspackages/vinext/src/shims/error-boundary.tsx(consolidate duplicateisRedirectErrorvia import from./navigation.js)packages/vinext/src/shims/next-shims.d.ts(TS declarations for the new exports)packages/vinext/src/utils/navigation-signal.ts(drop unusedgetErrorDigestexport after consolidation)tests/shims.test.tsTests
22 new test cases in
tests/shims.test.ts:unstable_rethrowrethrows redirect, notFound, forbidden, unauthorized; no-op for unrelated errors; recurses througherror.cause.BailoutToCSRError+DynamicServerErrorconstructors / predicates (canonical digests, foreign-object detection by digest, negative cases).unstable_rethrowpropagatesBailoutToCSRErrorandDynamicServerError.unstable_rethrowdoes NOT match the four uncovered server-only categories — pins the intentional gap.unstable_isUnrecognizedActionErrorpredicate behavior.navigation.react-serverre-exportsunstable_rethrowand stubsunstable_isUnrecognizedActionError.unstable_catchErrorexposes a function; renders children when no error; renders the fallback withErrorInfo; re-throws Next.js router errors fromgetDerivedStateFromError; carries the expecteddisplayName.unstable_catchErroracceptsnull/undefined/ non-Error thrown values (ported fromtest/e2e/app-dir/catch-error/throw-nullandthrow-undefined).unstable_retrythrows "client-only" on the server.unstable_retryon the client callsappRouterInstance.refresh()+ resets the boundary state.vp test run tests/shims.test.ts tests/error-boundary.test.ts tests/app-router.test.ts tests/app-rsc-errors.test.ts— 1274/1274 pass.vp check— clean (format + lint + type + knip).Follow-ups (intentionally out of scope)
isPostpone/isDynamicPostpone/isHangingPromiseRejectionError/isPrerenderInterruptedError— server-only Next.js internals tied to PPR / prerender machinery vinext doesn't implement. Will need them only if vinext grows PPR.unstable_retryerror message (unstable_retry() can only be used in the App Router. Use reset() in the Pages Router.) — requires readingPagesRouterContextfrom inside the boundary, which vinext'sunstable_catchErrordoesn't currently do.unstable_catchErrorthrows when called. We keep a single working implementation across both environments so SSR-only bundles don't break at module load — misuse in a Server Component will fail at render time. Splitting into per-environment shims is tracked separately.handleHardNavError,handleISRErrorpaths in the catch-error boundary.Next.js test fixtures consulted
test/e2e/app-dir/catch-error/(unstable_catchError fixture + tests, especiallythrow-null/throw-undefined)test/e2e/app-dir/unstable-rethrow/(cause, dynamic-error, not-found-page, redirect fixtures)test/e2e/app-dir/app-static/lib/fetch-retry.js(canonical user-facingunstable_rethrowuse case)🤖 Generated with Claude Code via opencode