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.

Sign up for GitHub

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

fix: ensure route handlers properly track dynamic access #66446

Merged
merged 2 commits into from
Jun 1, 2024
Merged

fix: ensure route handlers properly track dynamic access #66446

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d748396
fix: properly track dynamic access in route handlers
ztanner Jun 1, 2024
abba6c8
remove duplicate check
ztanner Jun 1, 2024
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
2 changes: 1 addition & 1 deletion packages/next/src/server/app-render/dynamic-rendering.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ export function trackDynamicDataAccessed(
if (store.isStaticGeneration) {
// We aren't prerendering but we are generating a static page. We need to bail out of static generation
const err = new DynamicServerError(
`Route ${pathname} couldn't be rendered statically because it used ${expression}. See more info here: https://nextjs.org/docs/messages/dynamic-server-error`
`Route ${pathname} couldn't be rendered statically because it used \`${expression}\`. See more info here: https://nextjs.org/docs/messages/dynamic-server-error`
)
store.dynamicUsageDescription = expression
store.dynamicUsageStack = err.stack
Expand Down
130 changes: 65 additions & 65 deletions packages/next/src/server/future/route-modules/app-route/module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,18 @@ import * as serverHooks from '../../../../client/components/hooks-server-context
import { DynamicServerError } from '../../../../client/components/hooks-server-context'

import { requestAsyncStorage } from '../../../../client/components/request-async-storage.external'
import { staticGenerationAsyncStorage } from '../../../../client/components/static-generation-async-storage.external'
import {
staticGenerationAsyncStorage,
type StaticGenerationStore,
} from '../../../../client/components/static-generation-async-storage.external'
import { actionAsyncStorage } from '../../../../client/components/action-async-storage.external'
import * as sharedModules from './shared-modules'
import { getIsServerAction } from '../../../lib/server-action-request-meta'
import { RequestCookies } from 'next/dist/compiled/@edge-runtime/cookies'
import { cleanURL } from './helpers/clean-url'
import { StaticGenBailoutError } from '../../../../client/components/static-generation-bailout'
import { isStaticGenEnabled } from './helpers/is-static-gen-enabled'
import { trackDynamicDataAccessed } from '../../../app-render/dynamic-rendering'

/**
* The AppRouteModule is the type of the module exported by the bundled App
Expand Down Expand Up @@ -317,7 +321,6 @@ export class AppRouteRouteModule extends RouteModule<
let request = rawRequest

// Update the static generation store based on the dynamic property.
if (isStaticGeneration) {
switch (this.dynamic) {
case 'force-dynamic': {
// Routes of generated paths should be dynamic
Expand All @@ -330,10 +333,7 @@ export class AppRouteRouteModule extends RouteModule<
staticGenerationStore.forceStatic = true
// We also Proxy the request to replace dynamic data on the request
// with empty stubs to allow for safely executing as static
request = new Proxy(
rawRequest,
forceStaticRequestHandlers
)
request = new Proxy(rawRequest, forceStaticRequestHandlers)
break
case 'error':
// The dynamic property is set to error, so we should throw an
Expand All @@ -346,25 +346,12 @@ export class AppRouteRouteModule extends RouteModule<
)
break
default:
// When we are statically generating a route we want to bail out if anything dynamic
// is accessed. We only create this proxy in the staticGenerationCase because it is overhead
// for dynamic runtime executions
request = new Proxy(
// We proxy `NextRequest` to track dynamic access, and potentially bail out of static generation
request = proxyNextRequest(
rawRequest,
staticGenerationRequestHandlers
staticGenerationStore
)
}
} else {
// Generally if we are in a dynamic render we don't have to modify much however for
// force-static specifically we ensure the dynamic and static behavior is consistent
// by proxying the request in the same way in both cases
if (this.dynamic === 'force-static') {
// The dynamic property is set to force-static, so we should
// force the page to be static.
staticGenerationStore.forceStatic = true
request = new Proxy(rawRequest, forceStaticRequestHandlers)
}
}

// If the static generation store does not have a revalidate value
// set, then we should set it the revalidate value from the userland
Expand Down Expand Up @@ -672,7 +659,48 @@ const forceStaticNextUrlHandlers = {
},
}

const staticGenerationRequestHandlers = {
function proxyNextRequest(
request: NextRequest,
staticGenerationStore: StaticGenerationStore
) {
const nextUrlHandlers = {
get(
target: NextURL & UrlSymbolTarget,
prop: string | symbol,
receiver: any
): unknown {
switch (prop) {
case 'search':
case 'searchParams':
case 'url':
case 'href':
case 'toJSON':
case 'toString':
case 'origin': {
trackDynamicDataAccessed(staticGenerationStore, `nextUrl.${prop}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't it NextRequest.${prop}?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the proxy within a proxy 😆 for nextRequest.nextUrl. So those errors become nextUrl.pathname for example

const result = Reflect.get(target, prop, receiver)
if (typeof result === 'function') {
return result.bind(target)
}
return result
}
case 'clone':
return (
target[urlCloneSymbol] ||
(target[urlCloneSymbol] = () =>
new Proxy(target.clone(), nextUrlHandlers))
)
default:
const result = Reflect.get(target, prop, receiver)
if (typeof result === 'function') {
return result.bind(target)
}
return result
}
},
}

const nextRequestHandlers = {
get(
target: NextRequest & RequestSymbolTarget,
prop: string | symbol,
Expand All @@ -682,10 +710,7 @@ const staticGenerationRequestHandlers = {
case 'nextUrl':
return (
target[nextURLSymbol] ||
(target[nextURLSymbol] = new Proxy(
target.nextUrl,
staticGenerationNextUrlHandlers
))
(target[nextURLSymbol] = new Proxy(target.nextUrl, nextUrlHandlers))
)
case 'headers':
case 'cookies':
Expand All @@ -695,10 +720,15 @@ const staticGenerationRequestHandlers = {
case 'json':
case 'text':
case 'arrayBuffer':
case 'formData':
throw new DynamicServerError(
`Route ${target.nextUrl.pathname} couldn't be rendered statically because it accessed \`request.${prop}\`. See more info here: https://nextjs.org/docs/messages/dynamic-server-error`
)
case 'formData': {
trackDynamicDataAccessed(staticGenerationStore, `request.${prop}`)

const result = Reflect.get(target, prop, receiver)
if (typeof result === 'function') {
return result.bind(target)
}
return result
}
case 'clone':
return (
target[requestCloneSymbol] ||
Expand All @@ -712,7 +742,7 @@ const staticGenerationRequestHandlers = {
// to probably embed the static generation logic into the class itself removing the need
// for any kind of proxying
target.clone() as NextRequest,
staticGenerationRequestHandlers
nextRequestHandlers
))
)
default:
Expand All @@ -727,37 +757,7 @@ const staticGenerationRequestHandlers = {
// and will be ignored
}

const staticGenerationNextUrlHandlers = {
get(
target: NextURL & UrlSymbolTarget,
prop: string | symbol,
receiver: any
): unknown {
switch (prop) {
case 'search':
case 'searchParams':
case 'url':
case 'href':
case 'toJSON':
case 'toString':
case 'origin':
throw new DynamicServerError(
`Route ${target.pathname} couldn't be rendered statically because it accessed \`nextUrl.${prop}\`. See more info here: https://nextjs.org/docs/messages/dynamic-server-error`
)
case 'clone':
return (
target[urlCloneSymbol] ||
(target[urlCloneSymbol] = () =>
new Proxy(target.clone(), staticGenerationNextUrlHandlers))
)
default:
const result = Reflect.get(target, prop, receiver)
if (typeof result === 'function') {
return result.bind(target)
}
return result
}
},
return new Proxy(request, nextRequestHandlers)
}

const requireStaticRequestHandlers = {
Expand Down Expand Up @@ -785,7 +785,7 @@ const requireStaticRequestHandlers = {
case 'arrayBuffer':
case 'formData':
throw new StaticGenBailoutError(
`Route ${target.nextUrl.pathname} with \`dynamic = "error"\` couldn't be rendered statically because it accessed \`request.${prop}\`.`
`Route ${target.nextUrl.pathname} with \`dynamic = "error"\` couldn't be rendered statically because it used \`request.${prop}\`.`
)
case 'clone':
return (
Expand Down Expand Up @@ -830,7 +830,7 @@ const requireStaticNextUrlHandlers = {
case 'toString':
case 'origin':
throw new StaticGenBailoutError(
`Route ${target.pathname} with \`dynamic = "error"\` couldn't be rendered statically because it accessed \`nextUrl.${prop}\`.`
`Route ${target.pathname} with \`dynamic = "error"\` couldn't be rendered statically because it used \`nextUrl.${prop}\`.`
)
case 'clone':
return (
Expand Down
14 changes: 9 additions & 5 deletions packages/next/src/server/lib/patch-fetch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,11 +374,15 @@ function createPatchedFetcher(
getRequestMeta('method')?.toLowerCase() || 'get'
)

// if there are authorized headers or a POST method and
// dynamic data usage was present above the tree we bail
// e.g. if cookies() is used before an authed/POST fetch
// or no user provided fetch cache config or revalidate
// is provided we don't cache
/**
* We automatically disable fetch caching under the following conditions:
* - Fetch cache configs are not set. Specifically:
* - A page fetch cache mode is not set (export const fetchCache=...)
* - A fetch cache mode is not set in the fetch call (fetch(url, { cache: ... }))
* - A fetch revalidate value is not set in the fetch call (fetch(url, { revalidate: ... }))
* - OR the fetch comes after a configuration that triggered dynamic rendering (e.g., reading cookies())
* and the fetch was considered uncacheable (e.g., POST method or has authorization headers)
*/
const autoNoCache =
// this condition is hit for null/undefined
// eslint-disable-next-line eqeqeq
Expand Down
Loading
Loading