-
Notifications
You must be signed in to change notification settings - Fork 325
fix(server): render progressive action not-found HTML #1254
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
9d9b4e9
f2fbe74
3534c7c
2baed5b
a9b5437
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -85,6 +85,8 @@ type AppRscRouteMatch<TRoute> = { | |
| type DispatchMatchedPageOptions<TRoute> = { | ||
| cleanPathname: string; | ||
| formState: ReactFormState | null; | ||
| actionError?: unknown; | ||
| actionFailed?: boolean; | ||
| handlerStart: number; | ||
| interceptionContext: string | null; | ||
| isProgressiveActionRender: boolean; | ||
|
|
@@ -116,6 +118,18 @@ type HandleProgressiveActionRequestOptions = { | |
| request: Request; | ||
| }; | ||
|
|
||
| type ProgressiveActionFormStateResult = | ||
| | { | ||
| formState: ReactFormState | null; | ||
| kind: "form-state"; | ||
| } | ||
| | { | ||
| actionError: unknown; | ||
| actionFailed: true; | ||
| formState: null; | ||
| kind: "form-state"; | ||
| }; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: this type is structurally identical to Not blocking — just a maintenance observation. |
||
|
|
||
| type HandleServerActionRequestOptions = { | ||
| actionId: string | null; | ||
| cleanPathname: string; | ||
|
|
@@ -167,7 +181,7 @@ type CreateAppRscHandlerOptions<TRoute extends AppRscHandlerRoute> = { | |
| ensureInstrumentation?: () => Promise<void>; | ||
| handleProgressiveActionRequest: ( | ||
| options: HandleProgressiveActionRequestOptions, | ||
| ) => Promise<Response | { formState: ReactFormState | null; kind: "form-state" } | null>; | ||
| ) => Promise<Response | ProgressiveActionFormStateResult | null>; | ||
| handleServerActionRequest: ( | ||
| options: HandleServerActionRequestOptions, | ||
| ) => Promise<Response | null>; | ||
|
|
@@ -420,6 +434,12 @@ async function handleAppRscRequest<TRoute extends AppRscHandlerRoute>( | |
| if (progressiveActionResult instanceof Response) return progressiveActionResult; | ||
| const isProgressiveActionRender = progressiveActionResult?.kind === "form-state"; | ||
| const formState = isProgressiveActionRender ? progressiveActionResult.formState : null; | ||
| const failedProgressiveActionResult = | ||
| isProgressiveActionRender && "actionFailed" in progressiveActionResult | ||
| ? progressiveActionResult | ||
| : null; | ||
| const actionFailed = failedProgressiveActionResult !== null; | ||
| const actionError = failedProgressiveActionResult?.actionError; | ||
|
|
||
| const serverActionResponse = await options.handleServerActionRequest({ | ||
| actionId, | ||
|
|
@@ -521,6 +541,8 @@ async function handleAppRscRequest<TRoute extends AppRscHandlerRoute>( | |
| return options.dispatchMatchedPage({ | ||
| cleanPathname, | ||
| formState, | ||
| actionError, | ||
| actionFailed, | ||
| handlerStart, | ||
| interceptionContext: interceptionContextHeader, | ||
| isProgressiveActionRender, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -73,10 +73,17 @@ type AppServerActionRoute = { | |
| pattern: string; | ||
| }; | ||
|
|
||
| type ProgressiveServerActionResult = { | ||
| formState: ReactFormState | null; | ||
| kind: "form-state"; | ||
| }; | ||
| type ProgressiveServerActionResult = | ||
| | { | ||
| formState: ReactFormState | null; | ||
| kind: "form-state"; | ||
| } | ||
| | { | ||
| actionError: unknown; | ||
| actionFailed: true; | ||
| formState: null; | ||
| kind: "form-state"; | ||
| }; | ||
|
|
||
| type AppServerActionMatch<TRoute extends AppServerActionRoute> = { | ||
| params: AppPageParams; | ||
|
|
@@ -198,16 +205,6 @@ export type HandleServerActionRscRequestOptions< | |
| toInterceptOpts: (intercept: AppServerActionIntercept<TPage>) => TInterceptOpts; | ||
| }; | ||
|
|
||
| type ActionControlResponse = | ||
| | { | ||
| kind: "redirect"; | ||
| url: string; | ||
| } | ||
| | { | ||
| kind: "status"; | ||
| statusCode: number; | ||
| }; | ||
|
|
||
| /** | ||
| * Matches Next.js' server action argument cap to prevent stack overflow in | ||
| * Function.prototype.apply when decoding hostile action payloads. | ||
|
|
@@ -297,33 +294,6 @@ export async function readActionFormDataWithLimit( | |
| }).formData(); | ||
| } | ||
|
|
||
| function getActionControlResponse(error: unknown): ActionControlResponse | null { | ||
| const digest = getNextErrorDigest(error); | ||
| if (!digest) return null; | ||
|
|
||
| const redirect = parseNextRedirectDigest(digest); | ||
| if (redirect) { | ||
| return { | ||
| kind: "redirect", | ||
| url: redirect.url, | ||
| }; | ||
| } | ||
|
|
||
| const httpError = parseNextHttpErrorDigest(digest); | ||
| if (httpError) { | ||
| if (!Number.isInteger(httpError.status)) { | ||
| return null; | ||
| } | ||
|
|
||
| return { | ||
| kind: "status", | ||
| statusCode: httpError.status, | ||
| }; | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| function getActionRedirect(error: unknown): AppServerActionRedirect | null { | ||
| const digest = getNextErrorDigest(error); | ||
| if (!digest) return null; | ||
|
|
@@ -449,22 +419,43 @@ export async function handleProgressiveServerActionRequest( | |
| return null; | ||
| } | ||
|
|
||
| let actionControlResponse: ActionControlResponse | null = null; | ||
| let actionRedirect: AppServerActionRedirect | null = null; | ||
| let actionError: unknown = undefined; | ||
| let actionFailed = false; | ||
| let actionResult: unknown; | ||
| const previousHeadersPhase = options.setHeadersAccessPhase("action"); | ||
| try { | ||
| actionResult = await action(); | ||
| } catch (error) { | ||
| actionControlResponse = getActionControlResponse(error); | ||
| if (!actionControlResponse) { | ||
| throw error; | ||
| actionRedirect = getActionRedirect(error); | ||
| if (!actionRedirect) { | ||
| actionError = error; | ||
| actionFailed = true; | ||
| const isControlFlow = | ||
| getActionHttpFallbackStatus(error) !== null || isServerActionNotFoundError(error, null); | ||
| if (!isControlFlow) { | ||
| console.error("[vinext] Server action error:", error); | ||
| options.reportRequestError( | ||
| normalizeError(error), | ||
| { | ||
| path: options.cleanPathname, | ||
| method: options.request.method, | ||
| headers: Object.fromEntries(options.request.headers.entries()), | ||
| }, | ||
| { routerKind: "App Router", routePath: options.cleanPathname, routeType: "action" }, | ||
| ); | ||
| } | ||
|
Comment on lines
+432
to
+447
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The error reporting for non-control-flow errors is good. Worth noting: this means generic action errors are now reported twice — once here via This matches the spirit of the change (action execution owns reporting, page render owns rendering), but wanted to flag the double-report possibility in case it matters for observability. The existing Next.js behavior presumably has a similar characteristic since the action handler also logs before handing off. |
||
| } | ||
| } finally { | ||
| options.setHeadersAccessPhase(previousHeadersPhase); | ||
| } | ||
|
|
||
| if (!actionControlResponse) { | ||
| if (!actionRedirect) { | ||
| getAndClearActionRevalidationKind(); | ||
| if (actionFailed) { | ||
| return { kind: "form-state", formState: null, actionError, actionFailed }; | ||
| } | ||
|
|
||
| const formState = await options.decodeFormState(actionResult, body); | ||
| return { kind: "form-state", formState: formState ?? null }; | ||
| } | ||
|
|
@@ -477,9 +468,7 @@ export async function handleProgressiveServerActionRequest( | |
| options.clearRequestContext(); | ||
|
|
||
| const headers = new Headers(); | ||
| if (actionControlResponse.kind === "redirect") { | ||
| headers.set("Location", new URL(actionControlResponse.url, options.request.url).toString()); | ||
| } | ||
| headers.set("Location", new URL(actionRedirect.url, options.request.url).toString()); | ||
| mergeMiddlewareResponseHeaders(headers, options.middlewareHeaders); | ||
| for (const cookie of actionPendingCookies) { | ||
| headers.append("Set-Cookie", cookie); | ||
|
|
@@ -490,7 +479,7 @@ export async function handleProgressiveServerActionRequest( | |
| setActionRevalidatedHeader(headers, actionRevalidationKind); | ||
|
|
||
| return new Response(null, { | ||
| status: actionControlResponse.kind === "redirect" ? 303 : actionControlResponse.statusCode, | ||
| status: 303, | ||
| headers, | ||
| }); | ||
| } catch (error) { | ||
|
|
@@ -503,10 +492,7 @@ export async function handleProgressiveServerActionRequest( | |
|
|
||
| getAndClearActionRevalidationKind(); | ||
| options.getAndClearPendingCookies(); | ||
| // Next.js rethrows generic MPA action errors into its page render path. | ||
| // vinext does not yet implement that form-state render path, so unexpected | ||
| // action failures remain request failures here. | ||
| console.error("[vinext] Server action error:", error); | ||
| console.error("[vinext] Server action payload parsing error:", error); | ||
| options.reportRequestError( | ||
| normalizeError(error), | ||
| { | ||
|
|
@@ -520,7 +506,7 @@ export async function handleProgressiveServerActionRequest( | |
| return internalServerErrorResponse( | ||
| process.env.NODE_ENV === "production" | ||
| ? undefined | ||
| : "Server action failed: " + getServerActionFailureMessage(error), | ||
| : "Server action parsing failed: " + getServerActionFailureMessage(error), | ||
| ); | ||
| } | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't this be the same as
if options.actionError? I don't think we'd have a failure without an associated error?