Skip to content

Commit

Permalink
feat: support isr reason for on request error (#68845)
Browse files Browse the repository at this point in the history
### What

Provide `revalidateReason` in the `errorContext` argument of
`onRequestError` API

#### API

```ts
revalidateReason?: 'on-demand' | 'stale' | undefined
```

### Why

With this property, developer is able to determine if current error is
attached to an ISR request.

* `stale` means it revalidated due to the cache is expired
* `on-demand` means it revalidated due to a user initiated revalidation,
e.g. using `res.revalidate` to revalidate certain path

### Misc

* We don't generate edge entry of instrumentation when there's no edge
routes but instrumentation is presented

Note: for PPR case we'll tackle it later with another PPR specific task,
since we need to provide the render is a postpone render or just normal
render

Closes NDX-23
  • Loading branch information
huozhi authored Aug 14, 2024
1 parent d50e041 commit d714bb3
Show file tree
Hide file tree
Showing 22 changed files with 287 additions and 5 deletions.
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

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: {
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')
}
93 changes: 93 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,93 @@
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
}

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', () => {
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('/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('/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

0 comments on commit d714bb3

Please sign in to comment.