Skip to content
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

feat: support isr reason for on request error #68845

Merged
merged 14 commits into from
Aug 14, 2024
6 changes: 6 additions & 0 deletions packages/next/src/build/entries.ts
Original file line number Diff line number Diff line change
Expand Up @@ -792,6 +792,12 @@ export async function createEntrypoints(

await Promise.all(promises)

// Optimization: If there's only one instrumentation hook in edge compiler, which means there's no edge server entry.
// We remove the edge instrumentation entry from edge compiler as it can be pure server side.
if (edgeServer.instrumentation && Object.keys(edgeServer).length === 1) {
delete edgeServer.instrumentation
}

return {
client,
server,
Expand Down
1 change: 1 addition & 0 deletions packages/next/src/build/templates/middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ function errorHandledHandler(fn: AdapterOptions['handler']) {
routerKind: 'Pages Router',
routePath: '/middleware',
routeType: 'middleware',
revalidateReason: undefined,
}
)

Expand Down
1 change: 1 addition & 0 deletions packages/next/src/server/api-utils/node/api-resolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,7 @@ export async function apiResolver(
routerKind: 'Pages Router',
routePath: page || '',
routeType: 'route',
revalidateReason: undefined,
})

if (err instanceof ApiError) {
Expand Down
2 changes: 2 additions & 0 deletions packages/next/src/server/app-render/app-render.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ import { getServerActionRequestMetadata } from '../lib/server-action-request-met
import { createInitialRouterState } from '../../client/components/router-reducer/create-initial-router-state'
import { createMutableActionQueue } from '../../shared/lib/router/action-queue'
import { prerenderAsyncStorage } from './prerender-async-storage.external'
import { getRevalidateReason } from '../instrumentation/utils'

export type GetDynamicParamFromSegment = (
// [slug] / [[slug]] / [...slug]
Expand Down Expand Up @@ -420,6 +421,7 @@ function createErrorContext(
routePath: ctx.pagePath,
routeType: ctx.isAction ? 'action' : 'render',
renderSource,
revalidateReason: getRevalidateReason(ctx.staticGenerationStore),
}
}
/**
Expand Down
16 changes: 11 additions & 5 deletions packages/next/src/server/base-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ import {
} from './after/builtin-request-context'
import { ENCODED_TAGS } from './stream-utils/encodedTags'
import { NextRequestHint } from './web/adapter'
import { getRevalidateReason } from './instrumentation/utils'
import { RouteKind } from './route-kind'
import type { RouteModule } from './route-modules/route-module'

Expand Down Expand Up @@ -2520,17 +2521,18 @@ export default abstract class Server<
)
return null
} catch (err) {
// If this is during static generation, throw the error again.
if (isSSG) throw err

Log.error(err)

await this.instrumentationOnRequestError(err, req, {
routerKind: 'App Router',
routePath: pathname,
routeType: 'route',
revalidateReason: getRevalidateReason(renderOpts),
})

// If this is during static generation, throw the error again.
if (isSSG) throw err
huozhi marked this conversation as resolved.
Show resolved Hide resolved

Log.error(err)

// Otherwise, send a 500 response.
await sendResponse(req, res, handleInternalServerErrorResponse())

Expand Down Expand Up @@ -2579,6 +2581,10 @@ export default abstract class Server<
routerKind: 'Pages Router',
routePath: pathname,
routeType: 'render',
revalidateReason: getRevalidateReason({
isRevalidate: isSSG,
isOnDemandRevalidate: renderOpts.isOnDemandRevalidate,
}),
})
throw err
}
Expand Down
1 change: 1 addition & 0 deletions packages/next/src/server/instrumentation/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ export type RequestErrorContext = {
| 'react-server-components'
| 'react-server-components-payload'
| 'server-rendering'
revalidateReason: 'on-demand' | 'stale' | undefined
// TODO: other future instrumentation context
}

Expand Down
12 changes: 12 additions & 0 deletions packages/next/src/server/instrumentation/utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
export function getRevalidateReason(params: {
huozhi marked this conversation as resolved.
Show resolved Hide resolved
isOnDemandRevalidate?: boolean
isRevalidate?: boolean
}): 'on-demand' | 'stale' | undefined {
if (params.isOnDemandRevalidate) {
return 'on-demand'
}
if (params.isRevalidate) {
return 'stale'
}
return undefined
}
2 changes: 2 additions & 0 deletions packages/next/src/server/next-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -984,6 +984,8 @@ export default class NextNodeServer extends BaseServer<
routePath: match.definition.page,
routerKind: 'Pages Router',
routeType: 'route',
// Edge runtime does not support ISR
revalidateReason: undefined,
})
throw apiError
}
Expand Down
8 changes: 8 additions & 0 deletions test/e2e/on-request-error/isr/app/app/on-demand/page.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
export default function Page() {
if (process.env.NEXT_PHASE !== 'phase-production-build') {
throw new Error('app:on-demand')
}
return <p>{Date.now()}</p>
}

export const revalidate = 1000
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
export function GET() {
if (process.env.NEXT_PHASE !== 'phase-production-build') {
throw new Error('app:route:on-demand')
}
return new Response('app:route')
}

export const revalidate = 1000
8 changes: 8 additions & 0 deletions test/e2e/on-request-error/isr/app/app/route/stale/route.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
export function GET() {
if (process.env.NEXT_PHASE !== 'phase-production-build') {
throw new Error('app:route:stale')
}
return new Response('app:route')
}

export const revalidate = 2
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
'use server'

import { revalidatePath } from 'next/cache'

export async function revalidateSelf() {
revalidatePath('/app/self-revalidate')
}
21 changes: 21 additions & 0 deletions test/e2e/on-request-error/isr/app/app/self-revalidate/page.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
'use client'

import { revalidateSelf } from './action'

export default function Page() {
if (typeof window === 'undefined') {
if (process.env.NEXT_PHASE !== 'phase-production-build') {
throw new Error('app:self-revalidate')
}
}
return (
<button
onClick={() => {
revalidateSelf()
location.reload()
}}
>
revalidate
</button>
)
}
7 changes: 7 additions & 0 deletions test/e2e/on-request-error/isr/app/app/stale/page.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
export default function Page() {
if (process.env.NEXT_PHASE !== 'phase-production-build')
throw new Error('app:stale')
return <p>{Date.now()}</p>
}

export const revalidate = 2
12 changes: 12 additions & 0 deletions test/e2e/on-request-error/isr/app/layout.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
export const metadata = {
title: 'Next.js',
description: 'Generated by Next.js',
}

export default function RootLayout({ children }) {
return (
<html lang="en">
<body>{children}</body>
</html>
)
}
31 changes: 31 additions & 0 deletions test/e2e/on-request-error/isr/instrumentation.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import fs from 'fs'
import fsp from 'fs/promises'
import path from 'path'

const dir = path.dirname(new URL(import.meta.url).pathname)
const logPath = path.join(dir, 'output-log.json')

export async function register() {
await fsp.writeFile(logPath, '{}', 'utf8')
}

// Since only Node.js runtime support ISR, we can just write the error state to a file here.
// `onRequestError` will only be bundled within the Node.js runtime.
export async function onRequestError(err, request, context) {
const payload = {
message: err.message,
request,
context,
}

const json = fs.existsSync(logPath)
? JSON.parse(await fsp.readFile(logPath, 'utf8'))
: {}

json[payload.message] = payload

console.log(
`[instrumentation] write-log:${payload.message} ${payload.context.revalidateReason}`
)
await fsp.writeFile(logPath, JSON.stringify(json, null, 2), 'utf8')
}
95 changes: 95 additions & 0 deletions test/e2e/on-request-error/isr/isr.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
import { nextTestSetup } from 'e2e-utils'
import { retry, waitFor } from 'next-test-utils'
import { getOutputLogJson } from '../_testing/utils'

const outputLogPath = 'output-log.json'

describe('on-request-error - isr', () => {
const { next, skipped, isNextDev } = nextTestSetup({
files: __dirname,
skipDeployment: true,
})

if (skipped) {
return
}

if (isNextDev) {
it('should skip in development mode', () => {
// This ISR test is only applicable for production mode
})
return
}
huozhi marked this conversation as resolved.
Show resolved Hide resolved

async function matchRevalidateReason(
errorMessage: string,
revalidateReason: string
) {
await retry(async () => {
const json = await getOutputLogJson(next, outputLogPath)
expect(json[errorMessage]).toMatchObject({
context: {
revalidateReason,
},
})
})
}

describe('app router ISR', () => {
huozhi marked this conversation as resolved.
Show resolved Hide resolved
it('should capture correct reason for stale errored page', async () => {
await next.fetch('/app/stale')
await waitFor(2 * 1000) // wait for revalidation
await next.fetch('/app/stale')

await matchRevalidateReason('app:stale', 'stale')
})

it('should capture correct reason for on-demand revalidated page', async () => {
await next.fetch('/app/on-demand')
huozhi marked this conversation as resolved.
Show resolved Hide resolved
await next.fetch('/api/revalidate-path?path=/app/on-demand')

await matchRevalidateReason('app:on-demand', 'on-demand')
})

it('should capture correct reason for build errored route', async () => {
await next.fetch('/app/route/stale')
await waitFor(2 * 1000) // wait for revalidation
await next.fetch('/app/route/stale')

await matchRevalidateReason('app:route:stale', 'stale')
})

it('should capture correct reason for on-demand revalidated route', async () => {
await next.fetch('/api/revalidate-path?path=/app/route/on-demand')

await matchRevalidateReason('app:route:on-demand', 'on-demand')
})

it('should capture revalidate from server action', async () => {
const browser = await next.browser('/app/self-revalidate')
const button = await browser.elementByCss('button')
await button.click()

await retry(async () => {
await next.fetch('/app/self-revalidate')
await matchRevalidateReason('app:self-revalidate', 'stale')
})
})
})

describe('pages router ISR', () => {
it('should capture correct reason for stale errored page', async () => {
await next.fetch('/pages/stale')
await waitFor(2 * 1000) // wait for revalidation
await next.fetch('/pages/stale')

await matchRevalidateReason('pages:stale', 'stale')
})

it('should capture correct reason for on-demand revalidated page', async () => {
await next.fetch('/pages/on-demand')
huozhi marked this conversation as resolved.
Show resolved Hide resolved
await next.fetch('/api/revalidate-path?path=/pages/on-demand')
await matchRevalidateReason('pages:on-demand', 'on-demand')
})
})
})
5 changes: 5 additions & 0 deletions test/e2e/on-request-error/isr/next.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
module.exports = {
experimental: {
instrumentationHook: true,
},
}
11 changes: 11 additions & 0 deletions test/e2e/on-request-error/isr/pages/api/revalidate-path.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
export default async function handler(req, res) {
const { path } = req.query
try {
await res.revalidate(path)
return res.json({ revalidated: true })
} catch (err) {
console.error('Failed to revalidate:', err)
}

res.json({ revalidated: false })
}
16 changes: 16 additions & 0 deletions test/e2e/on-request-error/isr/pages/pages/on-demand.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
export default function Page() {
if (typeof window === 'undefined') {
if (process.env.NEXT_PHASE !== 'phase-production-build')
throw new Error('pages:on-demand')
}
return <p>{Date.now()}</p>
}

export async function getStaticProps() {
return {
props: {
key: 'value',
},
revalidate: 1000,
}
}
17 changes: 17 additions & 0 deletions test/e2e/on-request-error/isr/pages/pages/stale.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
export default function Page() {
if (typeof window === 'undefined') {
if (process.env.NEXT_PHASE !== 'phase-production-build')
throw new Error('pages:stale')
}

return <p>{Date.now()}</p>
}

export async function getStaticProps() {
return {
props: {
key: 'value',
},
revalidate: 2,
}
}
7 changes: 7 additions & 0 deletions test/ppr-tests-manifest.json
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,13 @@
"app dir - not found navigation - with overridden node env should be able to navigate to other page from root not-found page"
]
},
"test/e2e/on-request-error/isr/isr.test.ts": {
"failed": [
"on-request-error - isr app router ISR should capture correct reason for stale errored page",
"on-request-error - isr app router ISR should capture correct reason for on-demand revalidated page",
"on-request-error - isr app router ISR should capture revalidate from server action"
]
},
"test/e2e/opentelemetry/opentelemetry.test.ts": {
"failed": [
"opentelemetry root context app router should handle RSC with fetch",
Expand Down
Loading