fix(app-router): throw a typed error for unrecognized server actions#1250
Conversation
…odule `UnrecognizedActionError` and `unstable_isUnrecognizedActionError` were defined inline in the `next/navigation` shim. The client server-action dispatcher needs to throw that exact class for the predicate's `instanceof` check to recognize it, but `instanceof` is identity-based per module instance, and importing the whole navigation shim into the dispatcher would be wrong layering. Move both into a dependency-free `shims/unrecognized-action-error.ts`, mirroring Next.js's own `unrecognized-action-error.ts`. The navigation shim re-exports them, so the public `next/navigation` surface is unchanged. No behavior change; existing predicate tests still pass.
When a server action POST returns the `x-nextjs-action-not-found` response header, the App Router client dispatcher threw a plain `Error`. `unstable_isUnrecognizedActionError` is an `instanceof UnrecognizedActionError` check, so it returned `false` for the framework's own action-not-found path. Client `catch` blocks and error boundaries that rely on the predicate to detect client/server deployment skew never fired, which is the exact case the predicate exists for. Next.js's server-action reducer throws `new UnrecognizedActionError(...)` on this same header. vinext exposed the class and predicate but never constructed the error from its dispatcher. Add `throwOnServerActionNotFound`, the client-side counterpart of `createServerActionNotFoundResponse`, and call it from the client server-action callback so the thrown error is recognizable. The two internal helpers it subsumes are no longer exported. Tests feed the real server-emitted response through the helper and assert the public predicate recognizes the result.
commit: |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, well-motivated fix. The extraction into a shared zero-dependency module is the right call for the instanceof identity issue, and the test coverage directly validates the end-to-end predicate path (server response → client throw → public predicate).
Two minor observations:
-
Import style consistency (see inline comment): the new import in
server-action-not-found.tsuses the bare-specifier form without.js(matching the convention forvinext/shims/*imports inserver/), whilenavigation.tsuses the relative./unrecognized-action-error.jswith extension. Both resolve correctly — just noting the two styles coexist in the same PR. -
react-server condition gap (not introduced here, just awareness):
navigation.react-server.tsexports a throwing stub forunstable_isUnrecognizedActionErrorbut doesn't exportUnrecognizedActionErrorat all. If someone imports the class in a Server Component, they'll get a missing-export error rather than the helpful "client-only" message. Next.js behaves the same way (their react-server condition also omits the class), so this is parity — just calling it out since the class now lives in a standalone module that could be imported directly fromvinext/shims/unrecognized-action-errorin any environment.
Neither is blocking. Looks good.
| @@ -1,4 +1,5 @@ | |||
| import { NEXTJS_ACTION_NOT_FOUND_HEADER as SERVER_ACTION_NOT_FOUND_HEADER } from "./headers.js"; | |||
| import { UnrecognizedActionError } from "vinext/shims/unrecognized-action-error"; | |||
There was a problem hiding this comment.
Nit: every other vinext/shims/* import in this directory omits the extension (matching the bare-specifier convention for the package exports wildcard). This is consistent with those — just noting that navigation.ts uses the relative ./unrecognized-action-error.js form with the .js extension. Both resolve correctly, and both styles are used elsewhere, so no action needed.
|
Review submitted. Here's a summary of my assessment: Approved — this is a well-structured fix for a real bug where Key points:
|
2 participants
What this changes
vinext's App Router client server-action dispatcher now throws
UnrecognizedActionErrorinstead of a plainErrorwhen the server reports an unrecognized action id. The publicunstable_isUnrecognizedActionErrorpredicate now returnstruefor the framework's own action-not-found path, so clientcatchblocks and error boundaries can detect client/server deployment skew and recover (typically by reloading the page).Why
When a server action POST returns the
x-nextjs-action-not-foundresponse header, the client dispatcher inserver/app-browser-entry.tsthrewnew Error(...).unstable_isUnrecognizedActionErroris aninstanceof UnrecognizedActionErrorcheck, so it returnedfalsefor the one code path users actually hit.vinext exposed
UnrecognizedActionErrorand the predicate as public API, but never constructed the error from its own dispatcher. Its own JSDoc admitted this ("vinext does not yet construct this error from its server-action dispatcher"). The deployment-skew recovery API therefore worked only for hand-constructed errors, never the real framework failure.Next.js throws
new UnrecognizedActionError(...)on this exact header in its server-action reducer:https://github.com/vercel/next.js/blob/canary/packages/next/src/client/components/router-reducer/reducers/server-action-reducer.ts
Approach
UnrecognizedActionErrorandunstable_isUnrecognizedActionErrorinto a dependency-freeshims/unrecognized-action-error.ts, mirroring Next.js's ownunrecognized-action-error.ts.instanceofis identity-based per module instance, so the dispatcher and user code must resolve one shared class.navigation.tsre-exports both, keeping thenext/navigationpublic surface unchanged.throwOnServerActionNotFound(response, actionId)toserver/server-action-not-found.ts, the client-side counterpart of the existingcreateServerActionNotFoundResponse. The client server-action callback calls it.isServerActionNotFoundResponse,getServerActionNotFoundClientMessage) are no longer exported;throwOnServerActionNotFoundis the single boundary.Split into two commits: a pure refactor (extract the shared module) and the behavioral fix.
Non-goals: the no-JS / MPA form-action path (a server-rendered error page, not the client predicate) is unchanged.
navigation.react-server.tskeeps its existing client-only throwing stub.Validation
vp checkandknipclean.tests/app-server-action-execution.test.ts: feed the real server-emitted response (createServerActionNotFoundResponse()) throughthrowOnServerActionNotFound, assert the public predicate recognizes the result and the message names the stale action id, and assert a normal response does not throw.tests/shims.test.tspredicate tests still pass, confirming the re-export is intact.tests/app-router.test.tsandtests/entry-templates.test.tspass.Reference: Next.js e2e coverage for this behavior is
actions-unrecognized.test.ts.Risks / follow-ups
The one-line wiring in the client callback (
throwOnServerActionNotFound(fetchResponse, id)) is not unit-tested in isolation:app-browser-entry.tsself-executes hydration on import, and triggering a genuine action-not-found requires real client/server deployment skew, which a single fixture build cannot produce. The wiring is covered by the type checker and the existing server-actions E2E happy path. A full browser E2E mirroring Next.js's error-boundary test would need two-build skew infrastructure; called out here rather than faked with a mock cascade.