Skip to content

Commit

Permalink
Check for type of route handler returned value at build time (via the…
Browse files Browse the repository at this point in the history
… TS plugin) and at runtime (#51394)

### What?

Fixes #51130. Before this PR, the package assumes that route handlers return a `Response` which is not necessarily the case.

The linked issue specified three suggestions to resolve this

1. Return a default 200 response
2. Throw a better error message
3. or both

~~In this issue I implemented (3), except that it is a warning and not an error. Do tell if the team wants to follow a different approach, as it is not too hard to change this.~~

This PR implements (2).

### How?

The returned value of the handler is checked at runtime to ensure it is actually a `Response` instance.

The return type `AppRouteHandlerFn` is also modified to `unknown` to avoid similar assumptions elsewhere.

The TS plugin is also modified to check for the return type during build time.



Co-authored-by: JJ Kasper <22380829+ijjk@users.noreply.github.com>
  • Loading branch information
joulev and ijjk committed Sep 8, 2023
1 parent 6ffb8da commit f47c409
Show file tree
Hide file tree
Showing 5 changed files with 78 additions and 69 deletions.
13 changes: 13 additions & 0 deletions packages/next/src/build/webpack/plugins/next-types-plugin/index.ts
Expand Up @@ -107,6 +107,19 @@ if ('${method}' in entry) {
'${method}'
>
>()
checkFields<
Diff<
{
__tag__: '${method}',
__return_type__: Response | Promise<Response>
},
{
__tag__: '${method}',
__return_type__: ReturnType<MaybeField<TEntry, '${method}'>>
},
'${method}'
>
>()
}
`
).join('')
Expand Down
Expand Up @@ -70,7 +70,8 @@ type AppRouteHandlerFnContext = {
}

/**
* Handler function for app routes.
* Handler function for app routes. If a non-Response value is returned, an error
* will be thrown.
*/
export type AppRouteHandlerFn = (
/**
Expand All @@ -82,7 +83,7 @@ export type AppRouteHandlerFn = (
* dynamic route).
*/
ctx: AppRouteHandlerFnContext
) => Promise<Response> | Response
) => unknown

/**
* AppRouteHandlers describes the handlers for app routes that is provided by
Expand Down Expand Up @@ -368,6 +369,11 @@ export class AppRouteRouteModule extends RouteModule<
? parsedUrlQueryToParams(context.params)
: undefined,
})
if (!(res instanceof Response)) {
throw new Error(
`No response is returned from route handler '${this.resolvedPagePath}'. Ensure you return a \`Response\` or a \`NextResponse\` in all branches of your handler.`
)
}
;(context.staticGenerationContext as any).fetchMetrics =
staticGenerationStore.fetchMetrics

Expand Down

0 comments on commit f47c409

Please sign in to comment.