Check for type of route handler returned value at build time (via the 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>
This commit is contained in:
Vũ Văn Dũng
GitHub
parent
6ffb8dafbd
commit
f47c409174
5 changed files with 78 additions and 69 deletions
|
|
@ -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('')
|
||||
|
|
|
|||
|
|
@ -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 = (
|
||||
/**
|
||||
|
|
@ -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
|
||||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -8,7 +8,7 @@ import {
|
|||
cookieWithRequestMeta,
|
||||
} from './helpers'
|
||||
|
||||
const bathPath = process.env.BASE_PATH ?? ''
|
||||
const basePath = process.env.BASE_PATH ?? ''
|
||||
|
||||
createNextDescribe(
|
||||
'app-custom-routes',
|
||||
|
|
@ -27,7 +27,7 @@ createNextDescribe(
|
|||
).toBeTruthy()
|
||||
}
|
||||
expect(
|
||||
JSON.parse(await next.render(bathPath + '/api/hello.json'))
|
||||
JSON.parse(await next.render(basePath + '/api/hello.json'))
|
||||
).toEqual({
|
||||
pathname: '/api/hello.json',
|
||||
})
|
||||
|
|
@ -47,7 +47,7 @@ createNextDescribe(
|
|||
).toBeFalsy()
|
||||
}
|
||||
expect(
|
||||
JSON.parse(await next.render(bathPath + '/api/dynamic'))
|
||||
JSON.parse(await next.render(basePath + '/api/dynamic'))
|
||||
).toEqual({
|
||||
pathname: '/api/dynamic',
|
||||
query: {},
|
||||
|
|
@ -61,7 +61,7 @@ createNextDescribe(
|
|||
'/static/second/data.json',
|
||||
'/static/three/data.json',
|
||||
])('responds correctly on %s', async (path) => {
|
||||
expect(JSON.parse(await next.render(bathPath + path))).toEqual({
|
||||
expect(JSON.parse(await next.render(basePath + path))).toEqual({
|
||||
params: { slug: path.split('/')[2] },
|
||||
now: expect.any(Number),
|
||||
})
|
||||
|
|
@ -83,7 +83,7 @@ createNextDescribe(
|
|||
'/revalidate-1/second/data.json',
|
||||
'/revalidate-1/three/data.json',
|
||||
])('revalidates correctly on %s', async (path) => {
|
||||
const data = JSON.parse(await next.render(bathPath + path))
|
||||
const data = JSON.parse(await next.render(basePath + path))
|
||||
expect(data).toEqual({
|
||||
params: { slug: path.split('/')[2] },
|
||||
now: expect.any(Number),
|
||||
|
|
@ -91,7 +91,7 @@ createNextDescribe(
|
|||
|
||||
await check(async () => {
|
||||
expect(data).not.toEqual(
|
||||
JSON.parse(await next.render(bathPath + path))
|
||||
JSON.parse(await next.render(basePath + path))
|
||||
)
|
||||
return 'success'
|
||||
}, 'success')
|
||||
|
|
@ -117,7 +117,7 @@ createNextDescribe(
|
|||
it.each(['/basic/endpoint', '/basic/vercel/endpoint'])(
|
||||
'responds correctly on %s',
|
||||
async (path) => {
|
||||
const res = await next.fetch(bathPath + path, { method })
|
||||
const res = await next.fetch(basePath + path, { method })
|
||||
|
||||
expect(res.status).toEqual(200)
|
||||
expect(await res.text()).toContain('hello, world')
|
||||
|
|
@ -131,7 +131,7 @@ createNextDescribe(
|
|||
|
||||
describe('route groups', () => {
|
||||
it('routes to the correct handler', async () => {
|
||||
const res = await next.fetch(bathPath + '/basic/endpoint/nested')
|
||||
const res = await next.fetch(basePath + '/basic/endpoint/nested')
|
||||
|
||||
expect(res.status).toEqual(200)
|
||||
const meta = getRequestMeta(res.headers)
|
||||
|
|
@ -141,7 +141,7 @@ createNextDescribe(
|
|||
|
||||
describe('request', () => {
|
||||
it('can read query parameters', async () => {
|
||||
const res = await next.fetch(bathPath + '/advanced/query?ping=pong')
|
||||
const res = await next.fetch(basePath + '/advanced/query?ping=pong')
|
||||
|
||||
expect(res.status).toEqual(200)
|
||||
const meta = getRequestMeta(res.headers)
|
||||
|
|
@ -150,7 +150,7 @@ createNextDescribe(
|
|||
|
||||
it('can read query parameters (edge)', async () => {
|
||||
const res = await next.fetch(
|
||||
bathPath + '/edge/advanced/query?ping=pong'
|
||||
basePath + '/edge/advanced/query?ping=pong'
|
||||
)
|
||||
|
||||
expect(res.status).toEqual(200)
|
||||
|
|
@ -162,7 +162,7 @@ createNextDescribe(
|
|||
describe('response', () => {
|
||||
// TODO-APP: re-enable when rewrites are supported again
|
||||
it.skip('supports the NextResponse.rewrite() helper', async () => {
|
||||
const res = await next.fetch(bathPath + '/hooks/rewrite')
|
||||
const res = await next.fetch(basePath + '/hooks/rewrite')
|
||||
|
||||
expect(res.status).toEqual(200)
|
||||
|
||||
|
|
@ -173,7 +173,7 @@ createNextDescribe(
|
|||
})
|
||||
|
||||
it('supports the NextResponse.redirect() helper', async () => {
|
||||
const res = await next.fetch(bathPath + '/hooks/redirect/response', {
|
||||
const res = await next.fetch(basePath + '/hooks/redirect/response', {
|
||||
// "Manually" perform the redirect, we want to inspect the
|
||||
// redirection response, so don't actually follow it.
|
||||
redirect: 'manual',
|
||||
|
|
@ -186,7 +186,7 @@ createNextDescribe(
|
|||
|
||||
it('supports the NextResponse.json() helper', async () => {
|
||||
const meta = { ping: 'pong' }
|
||||
const res = await next.fetch(bathPath + '/hooks/json', {
|
||||
const res = await next.fetch(basePath + '/hooks/json', {
|
||||
headers: withRequestMeta(meta),
|
||||
})
|
||||
|
||||
|
|
@ -212,7 +212,7 @@ createNextDescribe(
|
|||
},
|
||||
})
|
||||
|
||||
const res = await next.fetch(bathPath + '/advanced/body/streaming', {
|
||||
const res = await next.fetch(basePath + '/advanced/body/streaming', {
|
||||
method: 'POST',
|
||||
body: stream,
|
||||
})
|
||||
|
|
@ -235,7 +235,7 @@ createNextDescribe(
|
|||
})
|
||||
|
||||
const res = await next.fetch(
|
||||
bathPath + '/edge/advanced/body/streaming',
|
||||
basePath + '/edge/advanced/body/streaming',
|
||||
{
|
||||
method: 'POST',
|
||||
body: stream,
|
||||
|
|
@ -248,7 +248,7 @@ createNextDescribe(
|
|||
|
||||
it('can read a JSON encoded body', async () => {
|
||||
const body = { ping: 'pong' }
|
||||
const res = await next.fetch(bathPath + '/advanced/body/json', {
|
||||
const res = await next.fetch(basePath + '/advanced/body/json', {
|
||||
method: 'POST',
|
||||
body: JSON.stringify(body),
|
||||
})
|
||||
|
|
@ -260,7 +260,7 @@ createNextDescribe(
|
|||
|
||||
it('can read a JSON encoded body (edge)', async () => {
|
||||
const body = { ping: 'pong' }
|
||||
const res = await next.fetch(bathPath + '/edge/advanced/body/json', {
|
||||
const res = await next.fetch(basePath + '/edge/advanced/body/json', {
|
||||
method: 'POST',
|
||||
body: JSON.stringify(body),
|
||||
})
|
||||
|
|
@ -272,7 +272,7 @@ createNextDescribe(
|
|||
|
||||
it('can read a JSON encoded body for DELETE requests', async () => {
|
||||
const body = { name: 'foo' }
|
||||
const res = await next.fetch(bathPath + '/advanced/body/json', {
|
||||
const res = await next.fetch(basePath + '/advanced/body/json', {
|
||||
method: 'DELETE',
|
||||
body: JSON.stringify(body),
|
||||
})
|
||||
|
|
@ -283,7 +283,7 @@ createNextDescribe(
|
|||
|
||||
it('can read a JSON encoded body for OPTIONS requests', async () => {
|
||||
const body = { name: 'bar' }
|
||||
const res = await next.fetch(bathPath + '/advanced/body/json', {
|
||||
const res = await next.fetch(basePath + '/advanced/body/json', {
|
||||
method: 'OPTIONS',
|
||||
body: JSON.stringify(body),
|
||||
})
|
||||
|
|
@ -306,7 +306,7 @@ createNextDescribe(
|
|||
index++
|
||||
},
|
||||
})
|
||||
const res = await next.fetch(bathPath + '/advanced/body/json', {
|
||||
const res = await next.fetch(basePath + '/advanced/body/json', {
|
||||
method: 'POST',
|
||||
body: stream,
|
||||
})
|
||||
|
|
@ -329,7 +329,7 @@ createNextDescribe(
|
|||
index++
|
||||
},
|
||||
})
|
||||
const res = await next.fetch(bathPath + '/edge/advanced/body/json', {
|
||||
const res = await next.fetch(basePath + '/edge/advanced/body/json', {
|
||||
method: 'POST',
|
||||
body: stream,
|
||||
})
|
||||
|
|
@ -341,7 +341,7 @@ createNextDescribe(
|
|||
|
||||
it('can read the text body', async () => {
|
||||
const body = 'hello, world'
|
||||
const res = await next.fetch(bathPath + '/advanced/body/text', {
|
||||
const res = await next.fetch(basePath + '/advanced/body/text', {
|
||||
method: 'POST',
|
||||
body,
|
||||
})
|
||||
|
|
@ -353,7 +353,7 @@ createNextDescribe(
|
|||
|
||||
it('can read the text body (edge)', async () => {
|
||||
const body = 'hello, world'
|
||||
const res = await next.fetch(bathPath + '/edge/advanced/body/text', {
|
||||
const res = await next.fetch(basePath + '/edge/advanced/body/text', {
|
||||
method: 'POST',
|
||||
body,
|
||||
})
|
||||
|
|
@ -366,7 +366,7 @@ createNextDescribe(
|
|||
|
||||
describe('context', () => {
|
||||
it('provides params to routes with dynamic parameters', async () => {
|
||||
const res = await next.fetch(bathPath + '/basic/vercel/endpoint')
|
||||
const res = await next.fetch(basePath + '/basic/vercel/endpoint')
|
||||
|
||||
expect(res.status).toEqual(200)
|
||||
const meta = getRequestMeta(res.headers)
|
||||
|
|
@ -375,7 +375,7 @@ createNextDescribe(
|
|||
|
||||
it('provides params to routes with catch-all routes', async () => {
|
||||
const res = await next.fetch(
|
||||
bathPath + '/basic/vercel/some/other/resource'
|
||||
basePath + '/basic/vercel/some/other/resource'
|
||||
)
|
||||
|
||||
expect(res.status).toEqual(200)
|
||||
|
|
@ -387,7 +387,7 @@ createNextDescribe(
|
|||
})
|
||||
|
||||
it('does not provide params to routes without dynamic parameters', async () => {
|
||||
const res = await next.fetch(bathPath + '/basic/endpoint')
|
||||
const res = await next.fetch(basePath + '/basic/endpoint')
|
||||
|
||||
expect(res.ok).toBeTrue()
|
||||
|
||||
|
|
@ -399,7 +399,7 @@ createNextDescribe(
|
|||
describe('hooks', () => {
|
||||
describe('headers', () => {
|
||||
it('gets the correct values', async () => {
|
||||
const res = await next.fetch(bathPath + '/hooks/headers', {
|
||||
const res = await next.fetch(basePath + '/hooks/headers', {
|
||||
headers: withRequestMeta({ ping: 'pong' }),
|
||||
})
|
||||
|
||||
|
|
@ -412,7 +412,7 @@ createNextDescribe(
|
|||
|
||||
describe('cookies', () => {
|
||||
it('gets the correct values', async () => {
|
||||
const res = await next.fetch(bathPath + '/hooks/cookies', {
|
||||
const res = await next.fetch(basePath + '/hooks/cookies', {
|
||||
headers: cookieWithRequestMeta({ ping: 'pong' }),
|
||||
})
|
||||
|
||||
|
|
@ -425,7 +425,7 @@ createNextDescribe(
|
|||
|
||||
describe('cookies().has()', () => {
|
||||
it('gets the correct values', async () => {
|
||||
const res = await next.fetch(bathPath + '/hooks/cookies/has')
|
||||
const res = await next.fetch(basePath + '/hooks/cookies/has')
|
||||
|
||||
expect(res.status).toEqual(200)
|
||||
|
||||
|
|
@ -435,7 +435,7 @@ createNextDescribe(
|
|||
|
||||
describe('redirect', () => {
|
||||
it('can respond correctly', async () => {
|
||||
const res = await next.fetch(bathPath + '/hooks/redirect', {
|
||||
const res = await next.fetch(basePath + '/hooks/redirect', {
|
||||
// "Manually" perform the redirect, we want to inspect the
|
||||
// redirection response, so don't actually follow it.
|
||||
redirect: 'manual',
|
||||
|
|
@ -449,7 +449,7 @@ createNextDescribe(
|
|||
|
||||
describe('notFound', () => {
|
||||
it('can respond correctly', async () => {
|
||||
const res = await next.fetch(bathPath + '/hooks/not-found')
|
||||
const res = await next.fetch(basePath + '/hooks/not-found')
|
||||
|
||||
expect(res.status).toEqual(404)
|
||||
expect(await res.text()).toBeEmpty()
|
||||
|
|
@ -459,7 +459,7 @@ createNextDescribe(
|
|||
|
||||
describe('error conditions', () => {
|
||||
it('responds with 400 (Bad Request) when the requested method is not a valid HTTP method', async () => {
|
||||
const res = await next.fetch(bathPath + '/status/405', {
|
||||
const res = await next.fetch(basePath + '/status/405', {
|
||||
method: 'HEADER',
|
||||
})
|
||||
|
||||
|
|
@ -468,7 +468,7 @@ createNextDescribe(
|
|||
})
|
||||
|
||||
it('responds with 405 (Method Not Allowed) when method is not implemented', async () => {
|
||||
const res = await next.fetch(bathPath + '/status/405', {
|
||||
const res = await next.fetch(basePath + '/status/405', {
|
||||
method: 'POST',
|
||||
})
|
||||
|
||||
|
|
@ -477,7 +477,7 @@ createNextDescribe(
|
|||
})
|
||||
|
||||
it('responds with 500 (Internal Server Error) when the handler throws an error', async () => {
|
||||
const res = await next.fetch(bathPath + '/status/500')
|
||||
const res = await next.fetch(basePath + '/status/500')
|
||||
|
||||
expect(res.status).toEqual(500)
|
||||
expect(await res.text()).toBeEmpty()
|
||||
|
|
@ -491,7 +491,7 @@ createNextDescribe(
|
|||
// testing that the specific route throws this error in the console.
|
||||
expect(next.cliOutput).not.toContain(error)
|
||||
|
||||
const res = await next.fetch(bathPath + '/status/500/next')
|
||||
const res = await next.fetch(basePath + '/status/500/next')
|
||||
|
||||
expect(res.status).toEqual(500)
|
||||
expect(await res.text()).toBeEmpty()
|
||||
|
|
@ -507,7 +507,7 @@ createNextDescribe(
|
|||
|
||||
describe('automatic implementations', () => {
|
||||
it('implements HEAD on routes with GET already implemented', async () => {
|
||||
const res = await next.fetch(bathPath + '/methods/head', {
|
||||
const res = await next.fetch(basePath + '/methods/head', {
|
||||
method: 'HEAD',
|
||||
})
|
||||
|
||||
|
|
@ -516,7 +516,7 @@ createNextDescribe(
|
|||
})
|
||||
|
||||
it('implements OPTIONS on routes', async () => {
|
||||
const res = await next.fetch(bathPath + '/methods/options', {
|
||||
const res = await next.fetch(basePath + '/methods/options', {
|
||||
method: 'OPTIONS',
|
||||
})
|
||||
|
||||
|
|
@ -531,7 +531,7 @@ createNextDescribe(
|
|||
|
||||
describe('edge functions', () => {
|
||||
it('returns response using edge runtime', async () => {
|
||||
const res = await next.fetch(bathPath + '/edge')
|
||||
const res = await next.fetch(basePath + '/edge')
|
||||
|
||||
expect(res.status).toEqual(200)
|
||||
expect(await res.text()).toContain('hello, world')
|
||||
|
|
@ -539,7 +539,7 @@ createNextDescribe(
|
|||
|
||||
it('returns a response when headers are accessed', async () => {
|
||||
const meta = { ping: 'pong' }
|
||||
const res = await next.fetch(bathPath + '/edge/headers', {
|
||||
const res = await next.fetch(basePath + '/edge/headers', {
|
||||
headers: withRequestMeta(meta),
|
||||
})
|
||||
|
||||
|
|
@ -550,7 +550,7 @@ createNextDescribe(
|
|||
|
||||
describe('dynamic = "force-static"', () => {
|
||||
it('strips search, headers, and domain from request', async () => {
|
||||
const res = await next.fetch(bathPath + '/dynamic?query=true', {
|
||||
const res = await next.fetch(basePath + '/dynamic?query=true', {
|
||||
headers: {
|
||||
accept: 'application/json',
|
||||
cookie: 'session=true',
|
||||
|
|
@ -579,7 +579,7 @@ createNextDescribe(
|
|||
|
||||
describe('customized metadata routes', () => {
|
||||
it('should work if conflict with metadata routes convention', async () => {
|
||||
const res = await next.fetch(bathPath + '/robots.txt')
|
||||
const res = await next.fetch(basePath + '/robots.txt')
|
||||
|
||||
expect(res.status).toEqual(200)
|
||||
expect(await res.text()).toBe(
|
||||
|
|
@ -601,7 +601,7 @@ createNextDescribe(
|
|||
])(
|
||||
'should print an error when using lowercase %p in dev',
|
||||
async (method: string) => {
|
||||
await next.fetch(bathPath + '/lowercase/' + method)
|
||||
await next.fetch(basePath + '/lowercase/' + method)
|
||||
|
||||
await check(() => {
|
||||
expect(next.cliOutput).toContain(
|
||||
|
|
@ -621,7 +621,7 @@ createNextDescribe(
|
|||
|
||||
describe('invalid exports', () => {
|
||||
it('should print an error when exporting a default handler in dev', async () => {
|
||||
const res = await next.fetch(bathPath + '/default')
|
||||
const res = await next.fetch(basePath + '/default')
|
||||
|
||||
// Ensure we get a 405 (Method Not Allowed) response when there is no
|
||||
// exported handler for the GET method.
|
||||
|
|
@ -639,5 +639,18 @@ createNextDescribe(
|
|||
})
|
||||
})
|
||||
}
|
||||
|
||||
describe('no response returned', () => {
|
||||
it('should print an error when no response is returned', async () => {
|
||||
await next.fetch(basePath + '/no-response', { method: 'POST' })
|
||||
|
||||
await check(() => {
|
||||
expect(next.cliOutput).toMatch(
|
||||
/No response is returned from route handler '.+\/route\.ts'\. Ensure you return a `Response` or a `NextResponse` in all branches of your handler\./
|
||||
)
|
||||
return 'yes'
|
||||
}, 'yes')
|
||||
})
|
||||
})
|
||||
}
|
||||
)
|
||||
|
|
|
|||
1
test/e2e/app-dir/app-routes/app/no-response/route.ts
Normal file
1
test/e2e/app-dir/app-routes/app/no-response/route.ts
Normal file
|
|
@ -0,0 +1 @@
|
|||
export function POST() {}
|
||||
|
|
@ -1,24 +0,0 @@
|
|||
{
|
||||
"compilerOptions": {
|
||||
"lib": ["dom", "dom.iterable", "esnext"],
|
||||
"allowJs": true,
|
||||
"skipLibCheck": true,
|
||||
"strict": false,
|
||||
"forceConsistentCasingInFileNames": true,
|
||||
"noEmit": true,
|
||||
"incremental": true,
|
||||
"esModuleInterop": true,
|
||||
"module": "esnext",
|
||||
"moduleResolution": "bundler",
|
||||
"resolveJsonModule": true,
|
||||
"isolatedModules": true,
|
||||
"jsx": "preserve",
|
||||
"plugins": [
|
||||
{
|
||||
"name": "next"
|
||||
}
|
||||
]
|
||||
},
|
||||
"include": ["next-env.d.ts", ".next/types/**/*.ts", "**/*.ts", "**/*.tsx"],
|
||||
"exclude": ["node_modules", "**/*.test.ts"]
|
||||
}
|
||||
Loading…
Reference in a new issue