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(after): allow request APIs in after (actions/handlers) #73345

Merged
merged 5 commits into from
Dec 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion packages/next/src/server/after/after-context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
workUnitAsyncStorage,
type WorkUnitStore,
} from '../app-render/work-unit-async-storage.external'
import { afterTaskAsyncStorage } from '../app-render/after-task-async-storage.external'

export type AfterContextOpts = {
isEnabled: boolean
Expand Down Expand Up @@ -70,6 +71,16 @@ export class AfterContext {
this.workUnitStores.add(workUnitStore)
}

const afterTaskStore = afterTaskAsyncStorage.getStore()

// This is used for checking if request APIs can be called inside `after`.
// Note that we need to check the phase in which the *topmost* `after` was called (which should be "action"),
// not the current phase (which might be "after" if we're in a nested after).
// Otherwise, we might allow `after(() => headers())`, but not `after(() => after(() => headers()))`.
const rootTaskSpawnPhase = afterTaskStore
? afterTaskStore.rootTaskSpawnPhase // nested after
: workUnitStore?.phase // topmost after

Comment on lines +74 to +83
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have great isolation already between action/render. If you put a non-blocking async function in an action and it is delayed enough that it doesn't run until the render phase has started you could get racey behavior where the speed of resolution determines the semantics of the after call. I think we decided this was ok b/c what useful thing could you do by spawning non-blockign async work in an action.

But I think you are right we need this new storage however unfortunate it might be though maybe it would be simpler to model it as a non-shadowing storage. Like conditionally run bindSnapshot inside a afterTaskAsyncStorage only at the top level when you are inside a store that is render phase. If there are any nested after calls they won't create deeper afterTask store instances because they're not in render phase so they'll naturally inherit the root phase

Copy link
Member Author

@lubieowoce lubieowoce Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you put a non-blocking async function in an action and it is delayed enough that it doesn't run until the render phase has started you could get racey behavior where the speed of resolution determines the semantics of the after call

sure yeah but i don't think we can do a lot about that, it's racey bc the user wrote racey code 🤷‍♀️

maybe it would be simpler to model it as a non-shadowing storage. [...] If there are any nested after calls they won't create deeper afterTask store instances

soon enough i'm going to shadow it anyway to pass along callstack information (in dev)
https://github.com/vercel/next.js/pull/73449/files#diff-d6514c30f67dede0884866e69aa5c1ce48326f9fa2d515b5eb5e7162b5b0eb14R107
(i need this so that we can stitch together a sensible stack for the after callback -- instead of just showing p-queue at the top, i want to tie it back to the place that initiated it)
so even if i do it like that for this PR, i'd need to change it in the next one

// this should only happen once.
if (!this.runCallbacksOnClosePromise) {
this.runCallbacksOnClosePromise = this.runCallbacksOnClose()
Expand All @@ -83,7 +94,9 @@ export class AfterContext {
// await x()
const wrappedCallback = bindSnapshot(async () => {
try {
await callback()
await afterTaskAsyncStorage.run({ rootTaskSpawnPhase }, () =>
wyattjoh marked this conversation as resolved.
Show resolved Hide resolved
callback()
)
} catch (error) {
this.reportTaskError(error)
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import type { AfterTaskAsyncStorage } from './after-task-async-storage.external'
import { createAsyncLocalStorage } from './async-local-storage'

export const afterTaskAsyncStorageInstance: AfterTaskAsyncStorage =
createAsyncLocalStorage()
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import type { AsyncLocalStorage } from 'async_hooks'

// Share the instance module in the next-shared layer
import { afterTaskAsyncStorageInstance as afterTaskAsyncStorage } from './after-task-async-storage-instance' with { 'turbopack-transition': 'next-shared' }
import type { WorkUnitStore } from './work-unit-async-storage.external'

export interface AfterTaskStore {
/** The phase in which the topmost `unstable_after` was called.
*
* NOTE: Can be undefined when running `generateStaticParams`,
* where we only have a `workStore`, no `workUnitStore`.
Comment on lines +10 to +11
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd argue that we should not permit after inside generateStaticParams at all. That seems like a bug.

Copy link
Member Author

@lubieowoce lubieowoce Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i guess maybe we merged #73217 while you were away? 😅

the use case here could be e.g.

  • metrics/logs: calling some content DB to get a list of posts and then uploading metrics related to that fetch somewhere
  • cleanups: calling some reusable fetch-related util that uses after for cleanups

i see no reason we should forbid it rly. if you feel strongly abt this we can revisit #73217 offline, but there's nothing new in this PR that's related to that, this is just a comment explaining why it might happen

*/
readonly rootTaskSpawnPhase: WorkUnitStore['phase'] | undefined
}

export type AfterTaskAsyncStorage = AsyncLocalStorage<AfterTaskStore>

export { afterTaskAsyncStorage }
7 changes: 6 additions & 1 deletion packages/next/src/server/request/connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
} from '../app-render/dynamic-rendering'
import { StaticGenBailoutError } from '../../client/components/static-generation-bailout'
import { makeHangingPromise } from '../dynamic-rendering-utils'
import { isRequestAPICallableInsideAfter } from './utils'

/**
* This function allows you to indicate that you require an actual user Request before continuing.
Expand All @@ -18,7 +19,11 @@ export function connection(): Promise<void> {
const workUnitStore = workUnitAsyncStorage.getStore()

if (workStore) {
if (workUnitStore && workUnitStore.phase === 'after') {
if (
workUnitStore &&
workUnitStore.phase === 'after' &&
!isRequestAPICallableInsideAfter()
) {
throw new Error(
`Route ${workStore.route} used "connection" inside "unstable_after(...)". The \`connection()\` function is used to indicate the subsequent code must only run when there is an actual Request, but "unstable_after(...)" executes after the request, so this function is not allowed in this scope. See more info here: https://nextjs.org/docs/canary/app/api-reference/functions/unstable_after`
)
Expand Down
7 changes: 6 additions & 1 deletion packages/next/src/server/request/cookies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import { StaticGenBailoutError } from '../../client/components/static-generation
import { makeHangingPromise } from '../dynamic-rendering-utils'
import { createDedupedByCallsiteServerErrorLoggerDev } from '../create-deduped-by-callsite-server-error-logger'
import { scheduleImmediate } from '../../lib/scheduler'
import { isRequestAPICallableInsideAfter } from './utils'

/**
* In this version of Next.js `cookies()` returns a Promise however you can still reference the properties of the underlying cookies object
Expand Down Expand Up @@ -52,7 +53,11 @@ export function cookies(): Promise<ReadonlyRequestCookies> {
const workUnitStore = workUnitAsyncStorage.getStore()

if (workStore) {
if (workUnitStore && workUnitStore.phase === 'after') {
if (
workUnitStore &&
workUnitStore.phase === 'after' &&
!isRequestAPICallableInsideAfter()
) {
throw new Error(
// TODO(after): clarify that this only applies to pages?
`Route ${workStore.route} used "cookies" inside "unstable_after(...)". This is not supported. If you need this data inside an "unstable_after" callback, use "cookies" outside of the callback. See more info here: https://nextjs.org/docs/canary/app/api-reference/functions/unstable_after`
Expand Down
7 changes: 6 additions & 1 deletion packages/next/src/server/request/headers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import { StaticGenBailoutError } from '../../client/components/static-generation
import { makeHangingPromise } from '../dynamic-rendering-utils'
import { createDedupedByCallsiteServerErrorLoggerDev } from '../create-deduped-by-callsite-server-error-logger'
import { scheduleImmediate } from '../../lib/scheduler'
import { isRequestAPICallableInsideAfter } from './utils'

/**
* In this version of Next.js `headers()` returns a Promise however you can still reference the properties of the underlying Headers instance
Expand Down Expand Up @@ -57,7 +58,11 @@ export function headers(): Promise<ReadonlyHeaders> {
const workUnitStore = workUnitAsyncStorage.getStore()

if (workStore) {
if (workUnitStore && workUnitStore.phase === 'after') {
if (
workUnitStore &&
workUnitStore.phase === 'after' &&
!isRequestAPICallableInsideAfter()
) {
throw new Error(
`Route ${workStore.route} used "headers" inside "unstable_after(...)". This is not supported. If you need this data inside an "unstable_after" callback, use "headers" outside of the callback. See more info here: https://nextjs.org/docs/canary/app/api-reference/functions/unstable_after`
)
Expand Down
6 changes: 6 additions & 0 deletions packages/next/src/server/request/utils.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { StaticGenBailoutError } from '../../client/components/static-generation-bailout'
import { afterTaskAsyncStorage } from '../app-render/after-task-async-storage.external'

// This regex will have fast negatives meaning valid identifiers may not pass
// this test. However this is only used during static generation to provide hints
Expand Down Expand Up @@ -40,6 +41,11 @@ export function throwWithStaticGenerationBailoutErrorWithDynamicError(
)
}

export function isRequestAPICallableInsideAfter() {
const afterTaskStore = afterTaskAsyncStorage.getStore()
return afterTaskStore?.rootTaskSpawnPhase === 'action'
}

export const wellKnownProperties = new Set([
'hasOwnProperty',
'isPrototypeOf',
Expand Down
Original file line number Diff line number Diff line change
@@ -1,31 +1,64 @@
import { cookies, headers } from 'next/headers'
import { unstable_after as after, connection } from 'next/server'

export function testRequestAPIs() {
export function testRequestAPIs(/** @type {string} */ route) {
after(async () => {
try {
await headers()
console.log('headers(): ok')
console.log(`[${route}] headers(): ok`)
} catch (err) {
console.error(err)
console.error(`[${route}] headers(): error:`, err)
}
})

after(() =>
after(async () => {
try {
await headers()
console.log(`[${route}] nested headers(): ok`)
} catch (err) {
console.error(`[${route}] nested headers(): error:`, err)
}
})
)

after(async () => {
try {
await cookies()
console.log('cookies(): ok')
console.log(`[${route}] cookies(): ok`)
} catch (err) {
console.error(err)
console.error(`[${route}] cookies(): error:`, err)
}
})

after(() =>
after(async () => {
try {
await cookies()
console.log(`[${route}] nested cookies(): ok`)
} catch (err) {
console.error(`[${route}] nested cookies(): error:`, err)
}
})
)

after(async () => {
try {
await connection()
console.log('connection(): ok')
console.log(`[${route}] connection(): ok`)
} catch (err) {
console.error(err)
console.error(`[${route}] connection(): error:`, err)
}
})

after(() =>
after(async () => {
try {
await connection()
console.log(`[${route}] nested connection(): ok`)
} catch (err) {
console.error(`[${route}] nested connection(): error:`, err)
}
})
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,6 @@ import { testRequestAPIs } from '../helpers'
export const dynamic = 'error'

export default async function Page() {
testRequestAPIs()
testRequestAPIs('/request-apis/page-dynamic-error')
return null
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,6 @@ import { testRequestAPIs } from '../helpers'

export default async function Page() {
await connection()
testRequestAPIs()
testRequestAPIs('/request-apis/page-dynamic')
return null
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,6 @@ import { testRequestAPIs } from '../helpers'
export const dynamic = 'force-static'

export default async function Page() {
testRequestAPIs()
testRequestAPIs('/request-apis/page-force-static')
return null
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,6 @@ import { testRequestAPIs } from '../helpers'
export const dynamic = 'error'

export async function GET() {
testRequestAPIs()
testRequestAPIs('/request-apis/route-handler-dynamic-error')
return new Response()
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { testRequestAPIs } from '../helpers'

export async function GET() {
testRequestAPIs()
testRequestAPIs('/request-apis/route-handler-dynamic')
return new Response()
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,6 @@ import { testRequestAPIs } from '../helpers'
export const dynamic = 'force-static'

export async function GET() {
testRequestAPIs()
testRequestAPIs('/request-apis/route-handler-force-static')
return new Response()
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ export default async function Page() {
<form
action={async () => {
'use server'
testRequestAPIs()
testRequestAPIs('/request-apis/server-action')
}}
>
<button type="submit">Submit</button>
Expand Down
Loading
Loading