From a4763ccf14e8fd4ce08379c086e542d5f9f9d22b Mon Sep 17 00:00:00 2001 From: Zack Tanner <1939140+ztanner@users.noreply.github.com> Date: Thu, 3 Oct 2024 12:38:35 -0700 Subject: [PATCH 1/4] Reapply "Dedupe sync access warning on the Server by callsite" (#70672) (#70738) Relands https://github.com/vercel/next.js/pull/70672 with fixes that were failing in CI. [x-ref](https://github.com/vercel/next.js/actions/runs/11153352206/job/31002090099) --- ...-deduped-by-callsite-server-error-loger.ts | 59 +++++ packages/next/src/server/request/cookies.ts | 28 ++- .../next/src/server/request/draft-mode.ts | 14 +- packages/next/src/server/request/headers.ts | 28 ++- packages/next/src/server/request/params.ts | 38 ++- .../next/src/server/request/search-params.ts | 30 ++- test/.gitignore | 1 + .../dynamic-io-warnings/app/layout.tsx | 7 + .../app/pages/cookies/page.tsx | 19 ++ .../app/pages/draftMode/page.tsx | 24 ++ .../app/pages/headers/page.tsx | 19 ++ .../app/pages/params/[slug]/page.tsx | 19 ++ .../app/pages/searchParams/page.tsx | 27 ++ .../dynamic-io.warnings.test.ts | 232 ++++++++++++++++++ .../dynamic-io-warnings/next.config.js | 0 .../dynamic-io/dynamic-io.params.test.ts | 20 -- .../dynamic-io/dynamic-io.search.test.ts | 22 -- 17 files changed, 484 insertions(+), 103 deletions(-) create mode 100644 packages/next/src/server/create-deduped-by-callsite-server-error-loger.ts create mode 100644 test/development/app-dir/dynamic-io-warnings/app/layout.tsx create mode 100644 test/development/app-dir/dynamic-io-warnings/app/pages/cookies/page.tsx create mode 100644 test/development/app-dir/dynamic-io-warnings/app/pages/draftMode/page.tsx create mode 100644 test/development/app-dir/dynamic-io-warnings/app/pages/headers/page.tsx create mode 100644 test/development/app-dir/dynamic-io-warnings/app/pages/params/[slug]/page.tsx create mode 100644 test/development/app-dir/dynamic-io-warnings/app/pages/searchParams/page.tsx create mode 100644 test/development/app-dir/dynamic-io-warnings/dynamic-io.warnings.test.ts create mode 100644 test/development/app-dir/dynamic-io-warnings/next.config.js diff --git a/packages/next/src/server/create-deduped-by-callsite-server-error-loger.ts b/packages/next/src/server/create-deduped-by-callsite-server-error-loger.ts new file mode 100644 index 0000000000000..4bd1f0a1e3b57 --- /dev/null +++ b/packages/next/src/server/create-deduped-by-callsite-server-error-loger.ts @@ -0,0 +1,59 @@ +import * as React from 'react' + +const errorRef: { current: null | string } = { current: null } + +// React.cache is currently only available in canary/experimental React channels. +const cache = + typeof React.cache === 'function' + ? React.cache + : (fn: (key: unknown) => void) => fn + +// We don't want to dedupe across requests. +// The developer might've just attempted to fix the warning so we should warn again if it still happens. +const flushCurrentErrorIfNew = cache( + // eslint-disable-next-line @typescript-eslint/no-unused-vars -- cache key + (key: unknown) => { + try { + console.error(errorRef.current) + } finally { + errorRef.current = null + } + } +) + +/** + * Creates a function that logs an error message that is deduped by the userland + * callsite. + * This requires no indirection between the call of this function and the userland + * callsite i.e. there's only a single library frame above this. + * Do not use on the Client where sourcemaps and ignore listing might be enabled. + * Only use that for warnings need a fix independent of the callstack. + * + * @param getMessage + * @returns + */ +export function createDedupedByCallsiteServerErrorLoggerDev( + getMessage: (...args: Args) => string +) { + return function logDedupedError(...args: Args) { + const message = getMessage(...args) + + if (process.env.NODE_ENV !== 'production') { + const callStackFrames = new Error().stack?.split('\n') + if (callStackFrames === undefined || callStackFrames.length < 4) { + console.error(message) + } else { + // Error: + // logDedupedError + // asyncApiBeingAccessedSynchronously + // + // TODO: This breaks if sourcemaps with ignore lists are enabled. + const key = callStackFrames[3] + errorRef.current = message + flushCurrentErrorIfNew(key) + } + } else { + console.error(message) + } + } +} diff --git a/packages/next/src/server/request/cookies.ts b/packages/next/src/server/request/cookies.ts index 7d771976b5e8d..33ffccff98171 100644 --- a/packages/next/src/server/request/cookies.ts +++ b/packages/next/src/server/request/cookies.ts @@ -22,6 +22,7 @@ import { actionAsyncStorage } from '../../client/components/action-async-storage import { StaticGenBailoutError } from '../../client/components/static-generation-bailout' import { makeResolvedReactPromise } from './utils' import { makeHangingPromise } from '../dynamic-rendering-utils' +import { createDedupedByCallsiteServerErrorLoggerDev } from '../create-deduped-by-callsite-server-error-loger' /** * In this version of Next.js `cookies()` returns a Promise however you can still reference the properties of the underlying cookies object @@ -521,25 +522,30 @@ const noop = () => {} const warnForSyncIteration = process.env .__NEXT_DISABLE_SYNC_DYNAMIC_API_WARNINGS ? noop - : function warnForSyncIteration(route?: string) { - const prefix = route ? ` In route ${route} ` : '' - console.error( - `${prefix}cookies were iterated over. ` + + : createDedupedByCallsiteServerErrorLoggerDev( + function getSyncIterationMessage(route?: string) { + const prefix = route ? ` In route ${route} ` : '' + return ( + `${prefix}cookies were iterated over. ` + `\`cookies()\` should be awaited before using its value. ` + `Learn more: https://nextjs.org/docs/messages/sync-dynamic-apis` - ) - } + ) + } + ) const warnForSyncAccess = process.env.__NEXT_DISABLE_SYNC_DYNAMIC_API_WARNINGS ? noop - : function warnForSyncAccess(route: undefined | string, expression: string) { + : createDedupedByCallsiteServerErrorLoggerDev(function getSyncAccessMessage( + route: undefined | string, + expression: string + ) { const prefix = route ? ` In route ${route} a ` : 'A ' - console.error( + return ( `${prefix}cookie property was accessed directly with \`${expression}\`. ` + - `\`cookies()\` should be awaited before using its value. ` + - `Learn more: https://nextjs.org/docs/messages/sync-dynamic-apis` + `\`cookies()\` should be awaited before using its value. ` + + `Learn more: https://nextjs.org/docs/messages/sync-dynamic-apis` ) - } + }) function polyfilledResponseCookiesIterator( this: ResponseCookies diff --git a/packages/next/src/server/request/draft-mode.ts b/packages/next/src/server/request/draft-mode.ts index 2150aadf405e7..b60e36910cbc9 100644 --- a/packages/next/src/server/request/draft-mode.ts +++ b/packages/next/src/server/request/draft-mode.ts @@ -5,6 +5,7 @@ import type { DraftModeProvider } from '../../server/async-storage/draft-mode-pr import { workAsyncStorage } from '../../client/components/work-async-storage.external' import { cacheAsyncStorage } from '../../server/app-render/cache-async-storage.external' import { trackDynamicDataAccessed } from '../app-render/dynamic-rendering' +import { createDedupedByCallsiteServerErrorLoggerDev } from '../create-deduped-by-callsite-server-error-loger' /** * In this version of Next.js `draftMode()` returns a Promise however you can still reference the properties of the underlying draftMode object @@ -167,11 +168,14 @@ const noop = () => {} const warnForSyncAccess = process.env.__NEXT_DISABLE_SYNC_DYNAMIC_API_WARNINGS ? noop - : function warnForSyncAccess(route: undefined | string, expression: string) { + : createDedupedByCallsiteServerErrorLoggerDev(function getSyncAccessWarning( + route: undefined | string, + expression: string + ) { const prefix = route ? ` In route ${route} a ` : 'A ' - console.error( + return ( `${prefix}\`draftMode()\` property was accessed directly with \`${expression}\`. ` + - `\`draftMode()\` should be awaited before using its value. ` + - `Learn more: https://nextjs.org/docs/messages/draft-mode-sync-access` + `\`draftMode()\` should be awaited before using its value. ` + + `Learn more: https://nextjs.org/docs/messages/draft-mode-sync-access` ) - } + }) diff --git a/packages/next/src/server/request/headers.ts b/packages/next/src/server/request/headers.ts index 8025acbfc5162..8a94313be211e 100644 --- a/packages/next/src/server/request/headers.ts +++ b/packages/next/src/server/request/headers.ts @@ -19,6 +19,7 @@ import { import { StaticGenBailoutError } from '../../client/components/static-generation-bailout' import { makeResolvedReactPromise } from './utils' import { makeHangingPromise } from '../dynamic-rendering-utils' +import { createDedupedByCallsiteServerErrorLoggerDev } from '../create-deduped-by-callsite-server-error-loger' /** * In this version of Next.js `headers()` returns a Promise however you can still reference the properties of the underlying Headers instance @@ -436,25 +437,30 @@ const noop = () => {} const warnForSyncIteration = process.env .__NEXT_DISABLE_SYNC_DYNAMIC_API_WARNINGS ? noop - : function warnForSyncIteration(route?: string) { - const prefix = route ? ` In route ${route} ` : '' - console.error( - `${prefix}headers were iterated over. ` + + : createDedupedByCallsiteServerErrorLoggerDev( + function getSyncIterationMessage(route?: string) { + const prefix = route ? ` In route ${route} ` : '' + return ( + `${prefix}headers were iterated over. ` + `\`headers()\` should be awaited before using its value. ` + `Learn more: https://nextjs.org/docs/messages/sync-dynamic-apis` - ) - } + ) + } + ) const warnForSyncAccess = process.env.__NEXT_DISABLE_SYNC_DYNAMIC_API_WARNINGS ? noop - : function warnForSyncAccess(route: undefined | string, expression: string) { + : createDedupedByCallsiteServerErrorLoggerDev(function getSyncAccessMessage( + route: undefined | string, + expression: string + ) { const prefix = route ? ` In route ${route} a ` : 'A ' - console.error( + return ( `${prefix}header property was accessed directly with \`${expression}\`. ` + - `\`headers()\` should be awaited before using its value. ` + - `Learn more: https://nextjs.org/docs/messages/sync-dynamic-apis` + `\`headers()\` should be awaited before using its value. ` + + `Learn more: https://nextjs.org/docs/messages/sync-dynamic-apis` ) - } + }) type HeadersExtensions = { [K in keyof ReadonlyHeaders]: unknown diff --git a/packages/next/src/server/request/params.ts b/packages/next/src/server/request/params.ts index b4ce8a3f80bee..e19b672e1fba8 100644 --- a/packages/next/src/server/request/params.ts +++ b/packages/next/src/server/request/params.ts @@ -17,6 +17,7 @@ import { import { InvariantError } from '../../shared/lib/invariant-error' import { makeResolvedReactPromise, describeStringPropertyAccess } from './utils' import { makeHangingPromise } from '../dynamic-rendering-utils' +import { createDedupedByCallsiteServerErrorLoggerDev } from '../create-deduped-by-callsite-server-error-loger' export type Params = Record | undefined> @@ -491,46 +492,41 @@ const noop = () => {} const warnForSyncAccess = process.env.__NEXT_DISABLE_SYNC_DYNAMIC_API_WARNINGS ? noop - : function warnForSyncAccess(route: undefined | string, expression: string) { - if (process.env.__NEXT_DISABLE_SYNC_DYNAMIC_API_WARNINGS) { - return - } - + : createDedupedByCallsiteServerErrorLoggerDev(function getSyncAccessMessage( + route: undefined | string, + expression: string + ) { const prefix = route ? ` In route ${route} a ` : 'A ' - console.error( + return ( `${prefix}param property was accessed directly with ${expression}. ` + - `\`params\` should be awaited before accessing its properties. ` + - `Learn more: https://nextjs.org/docs/messages/sync-dynamic-apis` + `\`params\` should be awaited before accessing its properties. ` + + `Learn more: https://nextjs.org/docs/messages/sync-dynamic-apis` ) - } + }) const warnForEnumeration = process.env.__NEXT_DISABLE_SYNC_DYNAMIC_API_WARNINGS ? noop - : function warnForEnumeration( + : createDedupedByCallsiteServerErrorLoggerDev(function getEnumerationMessage( route: undefined | string, missingProperties: Array ) { - if (process.env.__NEXT_DISABLE_SYNC_DYNAMIC_API_WARNINGS) { - return - } - const prefix = route ? ` In route ${route} ` : '' if (missingProperties.length) { const describedMissingProperties = describeListOfPropertyNames(missingProperties) - console.error( + return ( `${prefix}params are being enumerated incompletely missing these properties: ${describedMissingProperties}. ` + - `\`params\` should be awaited before accessing its properties. ` + - `Learn more: https://nextjs.org/docs/messages/sync-dynamic-apis` + `\`params\` should be awaited before accessing its properties. ` + + `Learn more: https://nextjs.org/docs/messages/sync-dynamic-apis` ) } else { - console.error( + return ( `${prefix}params are being enumerated. ` + - `\`params\` should be awaited before accessing its properties. ` + - `Learn more: https://nextjs.org/docs/messages/sync-dynamic-apis` + `\`params\` should be awaited before accessing its properties. ` + + `Learn more: https://nextjs.org/docs/messages/sync-dynamic-apis` ) } - } + }) function describeListOfPropertyNames(properties: Array) { switch (properties.length) { diff --git a/packages/next/src/server/request/search-params.ts b/packages/next/src/server/request/search-params.ts index 2044b3e1b6c04..a00e22eaa8be3 100644 --- a/packages/next/src/server/request/search-params.ts +++ b/packages/next/src/server/request/search-params.ts @@ -18,6 +18,7 @@ import { import { cacheAsyncStorage } from '../app-render/cache-async-storage.external' import { InvariantError } from '../../shared/lib/invariant-error' import { makeHangingPromise } from '../dynamic-rendering-utils' +import { createDedupedByCallsiteServerErrorLoggerDev } from '../create-deduped-by-callsite-server-error-loger' import { describeStringPropertyAccess, describeHasCheckingStringProperty, @@ -660,18 +661,21 @@ const noop = () => {} const warnForSyncAccess = process.env.__NEXT_DISABLE_SYNC_DYNAMIC_API_WARNINGS ? noop - : function warnForSyncAccess(route: undefined | string, expression: string) { + : createDedupedByCallsiteServerErrorLoggerDev(function getSyncAccessMessage( + route: undefined | string, + expression: string + ) { const prefix = route ? ` In route ${route} a ` : 'A ' - console.error( + return ( `${prefix}searchParam property was accessed directly with ${expression}. ` + - `\`searchParams\` should be awaited before accessing properties. ` + - `Learn more: https://nextjs.org/docs/messages/sync-dynamic-apis` + `\`searchParams\` should be awaited before accessing properties. ` + + `Learn more: https://nextjs.org/docs/messages/sync-dynamic-apis` ) - } + }) const warnForEnumeration = process.env.__NEXT_DISABLE_SYNC_DYNAMIC_API_WARNINGS ? noop - : function warnForEnumeration( + : createDedupedByCallsiteServerErrorLoggerDev(function getEnumerationMessage( route: undefined | string, missingProperties: Array ) { @@ -679,19 +683,19 @@ const warnForEnumeration = process.env.__NEXT_DISABLE_SYNC_DYNAMIC_API_WARNINGS if (missingProperties.length) { const describedMissingProperties = describeListOfPropertyNames(missingProperties) - console.error( + return ( `${prefix}searchParams are being enumerated incompletely missing these properties: ${describedMissingProperties}. ` + - `\`searchParams\` should be awaited before accessing its properties. ` + - `Learn more: https://nextjs.org/docs/messages/sync-dynamic-apis` + `\`searchParams\` should be awaited before accessing its properties. ` + + `Learn more: https://nextjs.org/docs/messages/sync-dynamic-apis` ) } else { - console.error( + return ( `${prefix}searchParams are being enumerated. ` + - `\`searchParams\` should be awaited before accessing its properties. ` + - `Learn more: https://nextjs.org/docs/messages/sync-dynamic-apis` + `\`searchParams\` should be awaited before accessing its properties. ` + + `Learn more: https://nextjs.org/docs/messages/sync-dynamic-apis` ) } - } + }) function describeListOfPropertyNames(properties: Array) { switch (properties.length) { diff --git a/test/.gitignore b/test/.gitignore index cbdd2db2fff60..83e38f361cec1 100644 --- a/test/.gitignore +++ b/test/.gitignore @@ -3,6 +3,7 @@ e2e/**/tsconfig.json production/**/tsconfig.json +development/**/tsconfig.json test-junit-report/ turbopack-test-junit-report/ \ No newline at end of file diff --git a/test/development/app-dir/dynamic-io-warnings/app/layout.tsx b/test/development/app-dir/dynamic-io-warnings/app/layout.tsx new file mode 100644 index 0000000000000..e7077399c03ce --- /dev/null +++ b/test/development/app-dir/dynamic-io-warnings/app/layout.tsx @@ -0,0 +1,7 @@ +export default function Root({ children }: { children: React.ReactNode }) { + return ( + + {children} + + ) +} diff --git a/test/development/app-dir/dynamic-io-warnings/app/pages/cookies/page.tsx b/test/development/app-dir/dynamic-io-warnings/app/pages/cookies/page.tsx new file mode 100644 index 0000000000000..d919f55bd3153 --- /dev/null +++ b/test/development/app-dir/dynamic-io-warnings/app/pages/cookies/page.tsx @@ -0,0 +1,19 @@ +import { cookies, type UnsafeUnwrappedCookies } from 'next/headers' + +function Component() { + ;(cookies() as unknown as UnsafeUnwrappedCookies).get('component') + ;(cookies() as unknown as UnsafeUnwrappedCookies).has('component') + + const allCookies = [...(cookies() as unknown as UnsafeUnwrappedCookies)] + return 
{JSON.stringify(allCookies, null, 2)}
+} + +export default function Page() { + ;(cookies() as unknown as UnsafeUnwrappedCookies).get('page') + return ( + <> + + + + ) +} diff --git a/test/development/app-dir/dynamic-io-warnings/app/pages/draftMode/page.tsx b/test/development/app-dir/dynamic-io-warnings/app/pages/draftMode/page.tsx new file mode 100644 index 0000000000000..8ecb9e5733b86 --- /dev/null +++ b/test/development/app-dir/dynamic-io-warnings/app/pages/draftMode/page.tsx @@ -0,0 +1,24 @@ +import { draftMode, type UnsafeUnwrappedDraftMode } from 'next/headers' + +function Component() { + const isEnabled = (draftMode() as unknown as UnsafeUnwrappedDraftMode) + .isEnabled + ;(draftMode() as unknown as UnsafeUnwrappedDraftMode).enable() + + const clonedDraftMode = { + ...(draftMode() as unknown as UnsafeUnwrappedDraftMode), + } + return 
{JSON.stringify({ clonedDraftMode, isEnabled }, null, 2)}
+} + +export default function Page() { + const isEnabled = (draftMode() as unknown as UnsafeUnwrappedDraftMode) + .isEnabled + return ( + <> + 
{JSON.stringify({ isEnabled }, null, 2)}
+ + + + ) +} diff --git a/test/development/app-dir/dynamic-io-warnings/app/pages/headers/page.tsx b/test/development/app-dir/dynamic-io-warnings/app/pages/headers/page.tsx new file mode 100644 index 0000000000000..fe7dd562648fe --- /dev/null +++ b/test/development/app-dir/dynamic-io-warnings/app/pages/headers/page.tsx @@ -0,0 +1,19 @@ +import { headers, type UnsafeUnwrappedHeaders } from 'next/headers' + +function Component() { + ;(headers() as unknown as UnsafeUnwrappedHeaders).get('component') + ;(headers() as unknown as UnsafeUnwrappedHeaders).has('component') + + const allHeaders = [...(headers() as unknown as UnsafeUnwrappedHeaders)] + return 
{JSON.stringify(allHeaders, null, 2)}
+} + +export default function Page() { + ;(headers() as unknown as UnsafeUnwrappedHeaders).get('page') + return ( + <> + + + + ) +} diff --git a/test/development/app-dir/dynamic-io-warnings/app/pages/params/[slug]/page.tsx b/test/development/app-dir/dynamic-io-warnings/app/pages/params/[slug]/page.tsx new file mode 100644 index 0000000000000..b14e432c0de61 --- /dev/null +++ b/test/development/app-dir/dynamic-io-warnings/app/pages/params/[slug]/page.tsx @@ -0,0 +1,19 @@ +function Component({ params }: { params: { slug: string } }) { + const a = params.slug + const b = params.slug + + const clonedParams = { ...params } + return 
{JSON.stringify({ clonedParams, a, b }, null, 2)}
+} + +export default function Page({ params }: { params: { slug: string } }) { + const slug = params.slug + + return ( + <> + 
{JSON.stringify({ slug }, null, 2)}
+ + + + ) +} diff --git a/test/development/app-dir/dynamic-io-warnings/app/pages/searchParams/page.tsx b/test/development/app-dir/dynamic-io-warnings/app/pages/searchParams/page.tsx new file mode 100644 index 0000000000000..702091715c72d --- /dev/null +++ b/test/development/app-dir/dynamic-io-warnings/app/pages/searchParams/page.tsx @@ -0,0 +1,27 @@ +function Component({ + searchParams, +}: { + searchParams: Record +}) { + const a = searchParams.slug + const b = searchParams.slug + + const clonedSearchParams = { ...searchParams } + return 
{JSON.stringify({ clonedSearchParams, a, b }, null, 2)}
+} + +export default function Page({ + searchParams, +}: { + searchParams: Record +}) { + const slug = searchParams.slug + + return ( + <> + 
{JSON.stringify({ slug }, null, 2)}
+ + + + ) +} diff --git a/test/development/app-dir/dynamic-io-warnings/dynamic-io.warnings.test.ts b/test/development/app-dir/dynamic-io-warnings/dynamic-io.warnings.test.ts new file mode 100644 index 0000000000000..e606c69dca357 --- /dev/null +++ b/test/development/app-dir/dynamic-io-warnings/dynamic-io.warnings.test.ts @@ -0,0 +1,232 @@ +import { nextTestSetup } from 'e2e-utils' + +describe('dynamic-requests warnings', () => { + const { next } = nextTestSetup({ + files: __dirname, + }) + + it('warnings on sync cookie access', async () => { + const nextDevBootstrapOutputIndex = next.cliOutput.length + + const browser = await next.browser('/pages/cookies') + + const browserLogsserLogs = await browser.log() + const browserConsoleErrors = browserLogsserLogs + .filter((log) => log.source === 'error') + .map((log) => log.message) + const terminalOutput = next.cliOutput.slice(nextDevBootstrapOutputIndex) + const terminalCookieErrors = terminalOutput.split('\n').filter((line) => { + return line.includes('In route /pages/cookies') + }) + expect({ browserConsoleErrors, terminalCookieErrors }).toEqual({ + browserConsoleErrors: [ + expect.stringContaining( + "In route /pages/cookies a cookie property was accessed directly with `cookies().get('page')`." + ), + expect.stringContaining( + "In route /pages/cookies a cookie property was accessed directly with `cookies().get('component')`." + ), + expect.stringContaining( + "In route /pages/cookies a cookie property was accessed directly with `cookies().has('component')`." + ), + expect.stringContaining( + 'In route /pages/cookies cookies were iterated over' + ), + ], + terminalCookieErrors: [ + expect.stringContaining( + "In route /pages/cookies a cookie property was accessed directly with `cookies().get('page')`." + ), + expect.stringContaining( + "In route /pages/cookies a cookie property was accessed directly with `cookies().get('component')`." + ), + expect.stringContaining( + "In route /pages/cookies a cookie property was accessed directly with `cookies().has('component')`." + ), + expect.stringContaining( + 'In route /pages/cookies cookies were iterated over' + ), + ], + }) + }) + + it('warnings on sync draftMode access', async () => { + const nextDevBootstrapOutputIndex = next.cliOutput.length + + const browser = await next.browser('/pages/draftMode') + + const browserLogsserLogs = await browser.log() + const browserConsoleErrors = browserLogsserLogs + .filter((log) => log.source === 'error') + .map((log) => log.message) + const terminalOutput = next.cliOutput.slice(nextDevBootstrapOutputIndex) + const terminalCookieErrors = terminalOutput.split('\n').filter((line) => { + return line.includes('In route /pages/draftMode') + }) + expect({ browserConsoleErrors, terminalCookieErrors }).toEqual({ + browserConsoleErrors: [ + expect.stringContaining( + 'In route /pages/draftMode a `draftMode()` property was accessed directly with `draftMode().isEnabled`.' + ), + expect.stringContaining( + 'In route /pages/draftMode a `draftMode()` property was accessed directly with `draftMode().isEnabled`.' + ), + expect.stringContaining( + 'In route /pages/draftMode a `draftMode()` property was accessed directly with `draftMode().enable()`.' + ), + expect.stringContaining( + 'In route /pages/draftMode a `draftMode()` property was accessed directly with `draftMode().isEnabled`.' + ), + ], + terminalCookieErrors: [ + expect.stringContaining( + 'In route /pages/draftMode a `draftMode()` property was accessed directly with `draftMode().isEnabled`.' + ), + expect.stringContaining( + 'In route /pages/draftMode a `draftMode()` property was accessed directly with `draftMode().isEnabled`.' + ), + expect.stringContaining( + 'In route /pages/draftMode a `draftMode()` property was accessed directly with `draftMode().enable()`.' + ), + expect.stringContaining( + 'In route /pages/draftMode a `draftMode()` property was accessed directly with `draftMode().isEnabled`.' + ), + ], + }) + }) + + it('warnings on sync headers access', async () => { + const nextDevBootstrapOutputIndex = next.cliOutput.length + + const browser = await next.browser('/pages/headers') + + const browserLogsserLogs = await browser.log() + const browserConsoleErrors = browserLogsserLogs + .filter((log) => log.source === 'error') + .map((log) => log.message) + const terminalOutput = next.cliOutput.slice(nextDevBootstrapOutputIndex) + const terminalCookieErrors = terminalOutput.split('\n').filter((line) => { + return line.includes('In route /pages/headers') + }) + expect({ browserConsoleErrors, terminalCookieErrors }).toEqual({ + browserConsoleErrors: [ + expect.stringContaining( + "In route /pages/headers a header property was accessed directly with `headers().get('page')`." + ), + expect.stringContaining( + "In route /pages/headers a header property was accessed directly with `headers().get('component')`." + ), + expect.stringContaining( + "In route /pages/headers a header property was accessed directly with `headers().has('component')`." + ), + expect.stringContaining( + 'In route /pages/headers headers were iterated over' + ), + ], + terminalCookieErrors: [ + expect.stringContaining( + "In route /pages/headers a header property was accessed directly with `headers().get('page')`." + ), + expect.stringContaining( + "In route /pages/headers a header property was accessed directly with `headers().get('component')`." + ), + expect.stringContaining( + "In route /pages/headers a header property was accessed directly with `headers().has('component')`." + ), + expect.stringContaining( + 'In route /pages/headers headers were iterated over' + ), + ], + }) + }) + + it('warnings on sync params access', async () => { + const nextDevBootstrapOutputIndex = next.cliOutput.length + + const browser = await next.browser('/pages/params/[slug]') + + const browserLogsserLogs = await browser.log() + const browserConsoleErrors = browserLogsserLogs + .filter((log) => log.source === 'error') + .map((log) => log.message) + const terminalOutput = next.cliOutput.slice(nextDevBootstrapOutputIndex) + const terminalCookieErrors = terminalOutput.split('\n').filter((line) => { + return line.includes('In route /pages/params/[slug]') + }) + expect({ browserConsoleErrors, terminalCookieErrors }).toEqual({ + browserConsoleErrors: [ + expect.stringContaining( + 'In route /pages/params/[slug] a param property was accessed directly with `params.slug`.' + ), + expect.stringContaining( + 'In route /pages/params/[slug] a param property was accessed directly with `params.slug`.' + ), + expect.stringContaining( + 'In route /pages/params/[slug] a param property was accessed directly with `params.slug`.' + ), + expect.stringContaining( + 'In route /pages/params/[slug] params are being enumerated' + ), + ], + terminalCookieErrors: [ + expect.stringContaining( + 'In route /pages/params/[slug] a param property was accessed directly with `params.slug`.' + ), + expect.stringContaining( + 'In route /pages/params/[slug] a param property was accessed directly with `params.slug`.' + ), + expect.stringContaining( + 'In route /pages/params/[slug] a param property was accessed directly with `params.slug`.' + ), + expect.stringContaining( + 'In route /pages/params/[slug] params are being enumerated' + ), + ], + }) + }) + + it('warnings on sync searchParams access', async () => { + const nextDevBootstrapOutputIndex = next.cliOutput.length + + const browser = await next.browser('/pages/searchParams') + + const browserLogsserLogs = await browser.log() + const browserConsoleErrors = browserLogsserLogs + .filter((log) => log.source === 'error') + .map((log) => log.message) + const terminalOutput = next.cliOutput.slice(nextDevBootstrapOutputIndex) + const terminalCookieErrors = terminalOutput.split('\n').filter((line) => { + return line.includes('In route /pages/searchParams') + }) + expect({ browserConsoleErrors, terminalCookieErrors }).toEqual({ + browserConsoleErrors: [ + expect.stringContaining( + 'In route /pages/searchParams a searchParam property was accessed directly with `searchParams.slug`.' + ), + expect.stringContaining( + 'In route /pages/searchParams a searchParam property was accessed directly with `searchParams.slug`.' + ), + expect.stringContaining( + 'In route /pages/searchParams a searchParam property was accessed directly with `searchParams.slug`.' + ), + expect.stringContaining( + 'In route /pages/searchParams searchParams are being enumerated' + ), + ], + terminalCookieErrors: [ + expect.stringContaining( + 'In route /pages/searchParams a searchParam property was accessed directly with `searchParams.slug`.' + ), + expect.stringContaining( + 'In route /pages/searchParams a searchParam property was accessed directly with `searchParams.slug`.' + ), + expect.stringContaining( + 'In route /pages/searchParams a searchParam property was accessed directly with `searchParams.slug`.' + ), + expect.stringContaining( + 'In route /pages/searchParams searchParams are being enumerated' + ), + ], + }) + }) +}) diff --git a/test/development/app-dir/dynamic-io-warnings/next.config.js b/test/development/app-dir/dynamic-io-warnings/next.config.js new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/test/e2e/app-dir/dynamic-io/dynamic-io.params.test.ts b/test/e2e/app-dir/dynamic-io/dynamic-io.params.test.ts index 93383e7dc8203..64a31d27967d1 100644 --- a/test/e2e/app-dir/dynamic-io/dynamic-io.params.test.ts +++ b/test/e2e/app-dir/dynamic-io/dynamic-io.params.test.ts @@ -1773,8 +1773,6 @@ describe('dynamic-io', () => { expect($('#param-key-count').text()).toBe('2') expect(getLines('In route /params')).toEqual([ expect.stringContaining('params are being enumerated.'), - expect.stringContaining('accessed directly with `params.lowcard`'), - expect.stringContaining('accessed directly with `params.highcard`'), ]) } else { expect($('#layout').text()).toBe('at buildtime') @@ -1800,8 +1798,6 @@ describe('dynamic-io', () => { expect($('#param-key-count').text()).toBe('2') expect(getLines('In route /params')).toEqual([ expect.stringContaining('params are being enumerated.'), - expect.stringContaining('accessed directly with `params.lowcard`'), - expect.stringContaining('accessed directly with `params.highcard`'), ]) } else { expect($('#layout').text()).toBe('at buildtime') @@ -1827,8 +1823,6 @@ describe('dynamic-io', () => { expect($('#param-key-count').text()).toBe('2') expect(getLines('In route /params')).toEqual([ expect.stringContaining('params are being enumerated.'), - expect.stringContaining('accessed directly with `params.lowcard`'), - expect.stringContaining('accessed directly with `params.highcard`'), ]) } else { expect($('#layout').text()).toBe('at buildtime') @@ -1857,8 +1851,6 @@ describe('dynamic-io', () => { expect($('#param-key-count').text()).toBe('2') expect(getLines('In route /params')).toEqual([ expect.stringContaining('params are being enumerated.'), - expect.stringContaining('accessed directly with `params.lowcard`'), - expect.stringContaining('accessed directly with `params.highcard`'), ]) } else { expect($('#layout').text()).toBe('at buildtime') @@ -2007,8 +1999,6 @@ describe('dynamic-io', () => { expect($('#param-key-count').text()).toBe('2') expect(getLines('In route /params')).toEqual([ expect.stringContaining('params are being enumerated.'), - expect.stringContaining('accessed directly with `params.lowcard`'), - expect.stringContaining('accessed directly with `params.highcard`'), ]) } else { expect($('#layout').text()).toBe('at buildtime') @@ -2034,8 +2024,6 @@ describe('dynamic-io', () => { expect($('#param-key-count').text()).toBe('2') expect(getLines('In route /params')).toEqual([ expect.stringContaining('params are being enumerated.'), - expect.stringContaining('accessed directly with `params.lowcard`'), - expect.stringContaining('accessed directly with `params.highcard`'), ]) } else { expect($('#layout').text()).toBe('at buildtime') @@ -2124,8 +2112,6 @@ describe('dynamic-io', () => { expect($('#param-key-count').text()).toBe('2') expect(getLines('In route /params')).toEqual([ expect.stringContaining('params are being enumerated.'), - expect.stringContaining('accessed directly with `params.lowcard`'), - expect.stringContaining('accessed directly with `params.highcard`'), ]) } else { expect($('#layout').text()).toBe('at runtime') @@ -2152,8 +2138,6 @@ describe('dynamic-io', () => { expect($('#param-key-count').text()).toBe('2') expect(getLines('In route /params')).toEqual([ expect.stringContaining('params are being enumerated.'), - expect.stringContaining('accessed directly with `params.lowcard`'), - expect.stringContaining('accessed directly with `params.highcard`'), ]) } else { expect($('#layout').text()).toBe('at runtime') @@ -2339,8 +2323,6 @@ describe('dynamic-io', () => { ), expect.stringContaining('accessed directly with `params.dyn`'), expect.stringContaining('accessed directly with `params.value`'), - expect.stringContaining('accessed directly with `params.dyn`'), - expect.stringContaining('accessed directly with `params.value`'), ]) } else { expect($('#layout').text()).toBe('at runtime') @@ -2372,8 +2354,6 @@ describe('dynamic-io', () => { ), expect.stringContaining('accessed directly with `params.dyn`'), expect.stringContaining('accessed directly with `params.value`'), - expect.stringContaining('accessed directly with `params.dyn`'), - expect.stringContaining('accessed directly with `params.value`'), ]) } else { expect($('#layout').text()).toBe('at runtime') diff --git a/test/e2e/app-dir/dynamic-io/dynamic-io.search.test.ts b/test/e2e/app-dir/dynamic-io/dynamic-io.search.test.ts index abfd02fab589c..cfad2443702a7 100644 --- a/test/e2e/app-dir/dynamic-io/dynamic-io.search.test.ts +++ b/test/e2e/app-dir/dynamic-io/dynamic-io.search.test.ts @@ -397,13 +397,6 @@ describe('dynamic-io', () => { expect.stringContaining( 'searchParams are being enumerated incompletely' ), - expect.stringContaining( - 'accessed directly with `searchParams.sentinel`' - ), - expect.stringContaining('accessed directly with `searchParams.foo`'), - expect.stringContaining( - 'accessed directly with `searchParams.value`' - ), ]) } else { expect(searchWarnings).toHaveLength(0) @@ -431,10 +424,6 @@ describe('dynamic-io', () => { expect($('#page').text()).toBe('at runtime') expect(searchWarnings).toEqual([ expect.stringContaining('searchParams are being enumerated.'), - expect.stringContaining( - 'accessed directly with `searchParams.sentinel`' - ), - expect.stringContaining('accessed directly with `searchParams.foo`'), ]) } else { expect(searchWarnings).toHaveLength(0) @@ -705,13 +694,6 @@ describe('dynamic-io', () => { expect.stringContaining( 'searchParams are being enumerated incompletely' ), - expect.stringContaining( - 'accessed directly with `searchParams.sentinel`' - ), - expect.stringContaining('accessed directly with `searchParams.foo`'), - expect.stringContaining( - 'accessed directly with `searchParams.value`' - ), ]) } else { expect(searchWarnings).toHaveLength(0) @@ -735,10 +717,6 @@ describe('dynamic-io', () => { expect($('#page').text()).toBe('at runtime') expect(searchWarnings).toEqual([ expect.stringContaining('searchParams are being enumerated.'), - expect.stringContaining( - 'accessed directly with `searchParams.sentinel`' - ), - expect.stringContaining('accessed directly with `searchParams.foo`'), ]) } else { expect(searchWarnings).toHaveLength(0) From 08fce2e996c93a06ea00ccc0b790390cc7556d92 Mon Sep 17 00:00:00 2001 From: Shu Ding Date: Thu, 3 Oct 2024 23:35:22 +0200 Subject: [PATCH 2/4] Add RSC module ID mapping to the Client Manifest (#70524) Co-authored-by: Sebastian Markbage --- packages/next/src/build/webpack-config.ts | 3 +- .../loaders/next-flight-loader/index.ts | 44 ++++++++++--- .../plugins/flight-client-entry-plugin.ts | 17 +++++ .../webpack/plugins/flight-manifest-plugin.ts | 62 ++++++++++++++++++- .../next/src/server/app-render/encryption.ts | 15 ++--- .../src/server/use-cache/use-cache-wrapper.ts | 26 +++----- packages/next/types/$$compiled.internal.d.ts | 1 + test/e2e/app-dir/use-cache/app/client.tsx | 5 ++ test/e2e/app-dir/use-cache/app/page.tsx | 13 +++- test/e2e/app-dir/use-cache/use-cache.test.ts | 17 ++++- 10 files changed, 163 insertions(+), 40 deletions(-) create mode 100644 test/e2e/app-dir/use-cache/app/client.tsx diff --git a/packages/next/src/build/webpack-config.ts b/packages/next/src/build/webpack-config.ts index cf462c5af887c..fedf22331bec4 100644 --- a/packages/next/src/build/webpack-config.ts +++ b/packages/next/src/build/webpack-config.ts @@ -88,6 +88,7 @@ import { getBabelLoader, getReactCompilerLoader, } from './get-babel-loader-config' +import type { NextFlightLoaderOptions } from './webpack/loaders/next-flight-loader' type ExcludesFalse = (x: T | false) => x is T type ClientEntries = { @@ -525,7 +526,7 @@ export default async function getBaseWebpackConfig( loader: 'next-flight-loader', options: { isEdgeServer, - }, + } satisfies NextFlightLoaderOptions, } const appServerLayerLoaders = hasAppDir diff --git a/packages/next/src/build/webpack/loaders/next-flight-loader/index.ts b/packages/next/src/build/webpack/loaders/next-flight-loader/index.ts index 7792df2c3143b..3f9a43c2228e6 100644 --- a/packages/next/src/build/webpack/loaders/next-flight-loader/index.ts +++ b/packages/next/src/build/webpack/loaders/next-flight-loader/index.ts @@ -2,19 +2,35 @@ import type { webpack } from 'next/dist/compiled/webpack/webpack' import { RSC_MOD_REF_PROXY_ALIAS } from '../../../../lib/constants' import { BARREL_OPTIMIZATION_PREFIX, + DEFAULT_RUNTIME_WEBPACK, + EDGE_RUNTIME_WEBPACK, RSC_MODULE_TYPES, } from '../../../../shared/lib/constants' import { warnOnce } from '../../../../shared/lib/utils/warn-once' import { getRSCModuleInformation } from '../../../analysis/get-page-static-info' import { formatBarrelOptimizedResource } from '../../utils' import { getModuleBuildInfo } from '../get-module-build-info' +import type { + javascript, + LoaderContext, +} from 'next/dist/compiled/webpack/webpack' +import picomatch from 'next/dist/compiled/picomatch' + +export interface NextFlightLoaderOptions { + isEdgeServer: boolean +} + +type SourceType = javascript.JavascriptParser['sourceType'] | 'commonjs' const noopHeadPath = require.resolve('next/dist/client/components/noop-head') // For edge runtime it will be aliased to esm version by webpack const MODULE_PROXY_PATH = 'next/dist/build/webpack/loaders/next-flight-loader/module-proxy' -type SourceType = 'auto' | 'commonjs' | 'module' +const isSharedRuntime = picomatch('**/next/dist/**/*.shared-runtime.js', { + dot: true, // required for .pnpm paths +}) + export function getAssumedSourceType( mod: webpack.Module, sourceType: SourceType @@ -45,7 +61,7 @@ export function getAssumedSourceType( } export default function transformSource( - this: any, + this: LoaderContext, source: string, sourceMap: any ) { @@ -56,10 +72,11 @@ export default function transformSource( const options = this.getOptions() const { isEdgeServer } = options + const module = this._module! // Assign the RSC meta information to buildInfo. // Exclude next internal files which are not marked as client files - const buildInfo = getModuleBuildInfo(this._module) + const buildInfo = getModuleBuildInfo(module) buildInfo.rsc = getRSCModuleInformation(source, true) // Resource key is the unique identifier for the resource. When RSC renders @@ -75,19 +92,20 @@ export default function transformSource( // Because of that, we must add another query param to the resource key to // differentiate them. let resourceKey: string = this.resourcePath - if (this._module?.matchResource?.startsWith(BARREL_OPTIMIZATION_PREFIX)) { + if (module.matchResource?.startsWith(BARREL_OPTIMIZATION_PREFIX)) { resourceKey = formatBarrelOptimizedResource( resourceKey, - this._module.matchResource + module.matchResource ) } // A client boundary. if (buildInfo.rsc?.type === RSC_MODULE_TYPES.client) { const assumedSourceType = getAssumedSourceType( - this._module, - this._module?.parser?.sourceType + module, + (module.parser as javascript.JavascriptParser).sourceType ) + const clientRefs = buildInfo.rsc.clientRefs! if (assumedSourceType === 'module') { @@ -100,6 +118,18 @@ export default function transformSource( return } + if (!isSharedRuntime(resourceKey)) { + // Prevent module concatenation, and prevent export names from being + // mangled, in production builds, so that exports of client reference + // modules can be resolved by React using the metadata from the client + // manifest. + this._compilation!.moduleGraph.getExportsInfo( + module + ).setUsedInUnknownWay( + isEdgeServer ? EDGE_RUNTIME_WEBPACK : DEFAULT_RUNTIME_WEBPACK + ) + } + // `proxy` is the module proxy that we treat the module as a client boundary. // For ESM, we access the property of the module proxy directly for each export. // This is bit hacky that treating using a CJS like module proxy for ESM's exports, diff --git a/packages/next/src/build/webpack/plugins/flight-client-entry-plugin.ts b/packages/next/src/build/webpack/plugins/flight-client-entry-plugin.ts index 94229fce95772..13900a16db20e 100644 --- a/packages/next/src/build/webpack/plugins/flight-client-entry-plugin.ts +++ b/packages/next/src/build/webpack/plugins/flight-client-entry-plugin.ts @@ -94,6 +94,9 @@ const pluginState = getProxiedPluginState({ serverModuleIds: {} as Record, edgeServerModuleIds: {} as Record, + rscModuleIds: {} as Record, + edgeRscModuleIds: {} as Record, + injectedClientEntries: {} as Record, }) @@ -209,6 +212,20 @@ export class FlightClientEntryPlugin { : modPath + modQuery : mod.resource + if (typeof modId !== 'undefined' && modResource) { + if (mod.layer === WEBPACK_LAYERS.reactServerComponents) { + const key = path + .relative(compiler.context, modResource) + .replace(/\/next\/dist\/esm\//, '/next/dist/') + + if (this.isEdgeServer) { + pluginState.edgeRscModuleIds[key] = modId + } else { + pluginState.rscModuleIds[key] = modId + } + } + } + if (mod.layer !== WEBPACK_LAYERS.serverSideRendering) { return } diff --git a/packages/next/src/build/webpack/plugins/flight-manifest-plugin.ts b/packages/next/src/build/webpack/plugins/flight-manifest-plugin.ts index b1b5c6732fc3d..58f859be36a25 100644 --- a/packages/next/src/build/webpack/plugins/flight-manifest-plugin.ts +++ b/packages/next/src/build/webpack/plugins/flight-manifest-plugin.ts @@ -44,6 +44,9 @@ export type ManifestChunks = Array const pluginState = getProxiedPluginState({ serverModuleIds: {} as Record, edgeServerModuleIds: {} as Record, + + rscModuleIds: {} as Record, + edgeRscModuleIds: {} as Record, }) export interface ManifestNode { @@ -86,6 +89,12 @@ export type ClientReferenceManifest = { entryJSFiles?: { [entry: string]: string[] } + rscModuleMapping: { + [moduleId: string]: ManifestNode + } + edgeRscModuleMapping: { + [moduleId: string]: ManifestNode + } } function getAppPathRequiredChunks( @@ -173,6 +182,11 @@ function mergeManifest( manifestToMerge.edgeSSRModuleMapping ) Object.assign(manifest.entryCSSFiles, manifestToMerge.entryCSSFiles) + Object.assign(manifest.rscModuleMapping, manifestToMerge.rscModuleMapping) + Object.assign( + manifest.edgeRscModuleMapping, + manifestToMerge.edgeRscModuleMapping + ) } const PLUGIN_NAME = 'ClientReferenceManifestPlugin' @@ -268,6 +282,8 @@ export class ClientReferenceManifestPlugin { edgeSSRModuleMapping: {}, clientModules: {}, entryCSSFiles: {}, + rscModuleMapping: {}, + edgeRscModuleMapping: {}, } // Absolute path without the extension @@ -295,6 +311,9 @@ export class ClientReferenceManifestPlugin { const moduleIdMapping = manifest.ssrModuleMapping const edgeModuleIdMapping = manifest.edgeSSRModuleMapping + const rscIdMapping = manifest.rscModuleMapping + const edgeRscIdMapping = manifest.edgeRscModuleMapping + // Note that this isn't that reliable as webpack is still possible to assign // additional queries to make sure there's no conflict even using the `named` // module ID strategy. @@ -303,6 +322,11 @@ export class ClientReferenceManifestPlugin { mod.resourceResolveData?.path || resource ) + const rscNamedModuleId = relative( + context, + mod.resourceResolveData?.path || resource + ) + if (!ssrNamedModuleId.startsWith('.')) ssrNamedModuleId = `./${ssrNamedModuleId.replace(/\\/g, '/')}` @@ -345,7 +369,6 @@ export class ClientReferenceManifestPlugin { function addSSRIdMapping() { const exportName = resource if ( - // TODO: Add mapping from client module IDs to RSC module IDs typeof pluginState.serverModuleIds[ssrNamedModuleId] !== 'undefined' ) { moduleIdMapping[modId] = moduleIdMapping[modId] || {} @@ -375,12 +398,47 @@ export class ClientReferenceManifestPlugin { } } + function addRSCIdMapping() { + const exportName = resource + if ( + typeof pluginState.rscModuleIds[rscNamedModuleId] !== 'undefined' + ) { + rscIdMapping[modId] = rscIdMapping[modId] || {} + rscIdMapping[modId]['*'] = { + ...manifest.clientModules[exportName], + // During SSR, we don't have external chunks to load on the server + // side with our architecture of Webpack / Turbopack. We can keep + // this field empty to save some bytes. + chunks: [], + id: pluginState.rscModuleIds[rscNamedModuleId], + } + } + + if ( + typeof pluginState.edgeRscModuleIds[rscNamedModuleId] !== + 'undefined' + ) { + edgeRscIdMapping[modId] = edgeRscIdMapping[modId] || {} + edgeRscIdMapping[modId]['*'] = { + ...manifest.clientModules[exportName], + // During SSR, we don't have external chunks to load on the server + // side with our architecture of Webpack / Turbopack. We can keep + // this field empty to save some bytes. + chunks: [], + id: pluginState.edgeRscModuleIds[rscNamedModuleId], + } + } + } + addClientReference() addSSRIdMapping() + addRSCIdMapping() manifest.clientModules = moduleReferences manifest.ssrModuleMapping = moduleIdMapping manifest.edgeSSRModuleMapping = edgeModuleIdMapping + manifest.rscModuleMapping = rscIdMapping + manifest.edgeRscModuleMapping = edgeRscIdMapping } const checkedChunkGroups = new Set() @@ -479,6 +537,8 @@ export class ClientReferenceManifestPlugin { edgeSSRModuleMapping: {}, clientModules: {}, entryCSSFiles: {}, + rscModuleMapping: {}, + edgeRscModuleMapping: {}, } const segments = [...entryNameToGroupName(pageName).split('/'), 'page'] diff --git a/packages/next/src/server/app-render/encryption.ts b/packages/next/src/server/app-render/encryption.ts index 70f864c8ba415..862eef490fb7b 100644 --- a/packages/next/src/server/app-render/encryption.ts +++ b/packages/next/src/server/app-render/encryption.ts @@ -23,7 +23,7 @@ import { stringToUint8Array, } from './encryption-utils' -import type { ManifestNode } from '../../build/webpack/plugins/flight-manifest-plugin' +const isEdgeRuntime = process.env.NEXT_RUNTIME === 'edge' const textEncoder = new TextEncoder() const textDecoder = new TextDecoder() @@ -96,16 +96,11 @@ export async function decryptActionBoundArgs( actionId: string, encrypted: Promise ) { + const clientReferenceManifestSingleton = getClientReferenceManifestSingleton() + // Decrypt the serialized string with the action id as the salt. const decryped = await decodeActionBoundArg(actionId, await encrypted) - // TODO: We can't use the client reference manifest to resolve the modules - // on the server side - instead they need to be recovered as the module - // references (proxies) again. - // For now, we'll just use an empty module map. - const ssrModuleMap: { - [moduleExport: string]: ManifestNode - } = {} // Using Flight to deserialize the args from the string. const deserialized = await createFromReadableStream( new ReadableStream({ @@ -120,7 +115,9 @@ export async function decryptActionBoundArgs( // to be added to the current execution. Instead, we'll wait for any ClientReference // to be emitted which themselves will handle the preloading. moduleLoading: null, - moduleMap: ssrModuleMap, + moduleMap: isEdgeRuntime + ? clientReferenceManifestSingleton.edgeRscModuleMapping + : clientReferenceManifestSingleton.rscModuleMapping, }, } ) diff --git a/packages/next/src/server/use-cache/use-cache-wrapper.ts b/packages/next/src/server/use-cache/use-cache-wrapper.ts index 81e9e1e30478d..96b0e8849f07e 100644 --- a/packages/next/src/server/use-cache/use-cache-wrapper.ts +++ b/packages/next/src/server/use-cache/use-cache-wrapper.ts @@ -25,7 +25,7 @@ import { getServerModuleMap, } from '../app-render/encryption-utils' -import type { ManifestNode } from '../../build/webpack/plugins/flight-manifest-plugin' +const isEdgeRuntime = process.env.NEXT_RUNTIME === 'edge' type CacheEntry = { value: ReadableStream @@ -280,6 +280,11 @@ export function cache(kind: string, id: string, fn: any) { let entry: undefined | CacheEntry = await cacheHandler.get(serializedCacheKey) + // Get the clientReferenceManifestSingleton while we're still in the outer Context. + // In case getClientReferenceManifestSingleton is implemented using AsyncLocalStorage. + const clientReferenceManifestSingleton = + getClientReferenceManifestSingleton() + let stream if ( entry === undefined || @@ -297,11 +302,6 @@ export function cache(kind: string, id: string, fn: any) { // Note: It is important that we await at least once before this because it lets us // pop out of any stack specific contexts as well - aka "Sync" Local Storage. - // Get the clientReferenceManifestSingleton while we're still in the outer Context. - // In case getClientReferenceManifestSingleton is implemented using AsyncLocalStorage. - const clientReferenceManifestSingleton = - getClientReferenceManifestSingleton() - stream = await generateCacheEntry( workStore, clientReferenceManifestSingleton, @@ -315,8 +315,6 @@ export function cache(kind: string, id: string, fn: any) { if (entry.stale) { // If this is stale, and we're not in a prerender (i.e. this is dynamic render), // then we should warm up the cache with a fresh revalidated entry. - const clientReferenceManifestSingleton = - getClientReferenceManifestSingleton() const ignoredStream = await generateCacheEntry( workStore, clientReferenceManifestSingleton, @@ -338,20 +336,14 @@ export function cache(kind: string, id: string, fn: any) { // the server, which is required to pick it up for replaying again on the client. const replayConsoleLogs = true - // TODO: We can't use the client reference manifest to resolve the modules - // on the server side - instead they need to be recovered as the module - // references (proxies) again. - // For now, we'll just use an empty module map. - const ssrModuleMap: { - [moduleExport: string]: ManifestNode - } = {} - const ssrManifest = { // moduleLoading must be null because we don't want to trigger preloads of ClientReferences // to be added to the consumer. Instead, we'll wait for any ClientReference to be emitted // which themselves will handle the preloading. moduleLoading: null, - moduleMap: ssrModuleMap, + moduleMap: isEdgeRuntime + ? clientReferenceManifestSingleton.edgeRscModuleMapping + : clientReferenceManifestSingleton.rscModuleMapping, } return createFromReadableStream(stream, { ssrManifest, diff --git a/packages/next/types/$$compiled.internal.d.ts b/packages/next/types/$$compiled.internal.d.ts index ba1de837ef409..7b629c14609f9 100644 --- a/packages/next/types/$$compiled.internal.d.ts +++ b/packages/next/types/$$compiled.internal.d.ts @@ -561,6 +561,7 @@ declare module 'next/dist/compiled/webpack/webpack' { ModuleFilenameHelpers, } from 'webpack' export type { + javascript, LoaderDefinitionFunction, LoaderContext, ModuleGraph, diff --git a/test/e2e/app-dir/use-cache/app/client.tsx b/test/e2e/app-dir/use-cache/app/client.tsx new file mode 100644 index 0000000000000..87e5b5f029d6e --- /dev/null +++ b/test/e2e/app-dir/use-cache/app/client.tsx @@ -0,0 +1,5 @@ +'use client' + +export function Foo() { + return 'foo' +} diff --git a/test/e2e/app-dir/use-cache/app/page.tsx b/test/e2e/app-dir/use-cache/app/page.tsx index 09ba77896e320..ebe11f94e23a8 100644 --- a/test/e2e/app-dir/use-cache/app/page.tsx +++ b/test/e2e/app-dir/use-cache/app/page.tsx @@ -1,8 +1,12 @@ -async function getCachedRandom(x: number) { +import { Foo } from './client' + +async function getCachedRandom(x: number, children: React.ReactNode) { 'use cache' return { x, y: Math.random(), + z: , + r: children, } } @@ -12,11 +16,16 @@ export default async function Page({ searchParams: Promise<{ n: string }> }) { const n = +(await searchParams).n - const values = await getCachedRandom(n) + const values = await getCachedRandom( + n, +

rnd{Math.random()}

// This should not invalidate the cache + ) return ( <>

{values.x}

{values.y}

+

{values.z}

+ {values.r} ) } diff --git a/test/e2e/app-dir/use-cache/use-cache.test.ts b/test/e2e/app-dir/use-cache/use-cache.test.ts index d946bcb2f1037..a11b76d26726b 100644 --- a/test/e2e/app-dir/use-cache/use-cache.test.ts +++ b/test/e2e/app-dir/use-cache/use-cache.test.ts @@ -1,15 +1,20 @@ -// @ts-check +/* eslint-disable jest/no-standalone-expect */ import { nextTestSetup } from 'e2e-utils' const GENERIC_RSC_ERROR = 'An error occurred in the Server Components render. The specific message is omitted in production builds to avoid leaking sensitive details. A digest property is included on this error instance which may provide additional details about the nature of the error.' describe('use-cache', () => { - const { next, isNextDev, isNextDeploy } = nextTestSetup({ + const { next, isNextDev, isNextDeploy, isTurbopack } = nextTestSetup({ files: __dirname, }) - it('should cache results', async () => { + const itSkipTurbopack = isTurbopack ? it.skip : it + + // TODO: Fix the following error with Turbopack: + // Error: Module [project]/app/client.tsx [app-client] (ecmascript) was + // instantiated because it was required from module... + itSkipTurbopack('should cache results', async () => { const browser = await next.browser('/?n=1') expect(await browser.waitForElementByCss('#x').text()).toBe('1') const random1a = await browser.waitForElementByCss('#y').text() @@ -27,6 +32,12 @@ describe('use-cache', () => { // The navigation to n=2 should be some other random value. expect(random1a).not.toBe(random2) + + // Client component should have rendered. + expect(await browser.waitForElementByCss('#z').text()).toBe('foo') + + // Client component child should have rendered but not invalidated the cache. + expect(await browser.waitForElementByCss('#r').text()).toContain('rnd') }) it('should dedupe with react cache inside "use cache"', async () => { From 214c0a061ad8defdd03a764068b3a074b77e0ccc Mon Sep 17 00:00:00 2001 From: Benjamin Woodruff Date: Thu, 3 Oct 2024 15:01:37 -0700 Subject: [PATCH 3/4] feat(turbo-tasks): Add ResolvedVc versions of casting functions (#70575) We need a way to upcast/downcast `ResolvedVc` without an unnecessary conversion back to `Vc`. These are just wrappers of their `Vc` equivalents. --- .../crates/turbo-tasks/src/vc/resolved.rs | 75 ++++++++++++++++++- 1 file changed, 74 insertions(+), 1 deletion(-) diff --git a/turbopack/crates/turbo-tasks/src/vc/resolved.rs b/turbopack/crates/turbo-tasks/src/vc/resolved.rs index be126034f20ab..3f194faed1567 100644 --- a/turbopack/crates/turbo-tasks/src/vc/resolved.rs +++ b/turbopack/crates/turbo-tasks/src/vc/resolved.rs @@ -24,7 +24,7 @@ use serde::{Deserialize, Serialize}; use crate::{ trace::{TraceRawVcs, TraceRawVcsContext}, vc::Vc, - RcStr, VcRead, VcTransparentRead, VcValueType, + RcStr, ResolveTypeError, Upcast, VcRead, VcTransparentRead, VcValueTrait, VcValueType, }; #[derive(Serialize, Deserialize)] @@ -122,6 +122,79 @@ where } } +impl ResolvedVc +where + T: ?Sized + Send, +{ + /// Upcasts the given `ResolvedVc` to a `ResolvedVc>`. + /// + /// See also: [`Vc::upcast`]. + #[inline(always)] + pub fn upcast(this: Self) -> ResolvedVc + where + T: Upcast, + K: VcValueTrait + ?Sized + Send, + { + ResolvedVc { + node: Vc::upcast(this.node), + } + } +} + +impl ResolvedVc +where + T: VcValueTrait + ?Sized + Send, +{ + /// Attempts to sidecast the given `Vc>` to a `Vc>`. + /// + /// Returns `None` if the underlying value type does not implement `K`. + /// + /// **Note:** if the trait `T` is required to implement `K`, use [`ResolvedVc::upcast`] instead. + /// This provides stronger guarantees, removing the need for a [`Result`] return type. + /// + /// See also: [`Vc::try_resolve_sidecast`]. + pub async fn try_sidecast(this: Self) -> Result>, ResolveTypeError> + where + K: VcValueTrait + ?Sized + Send, + { + // must be async, as we must read the cell to determine the type + Ok(Vc::try_resolve_sidecast(this.node) + .await? + .map(|node| ResolvedVc { node })) + } + + /// Attempts to downcast the given `ResolvedVc>` to a `ResolvedVc`, where `K` + /// is of the form `Box`, and `L` is a value trait. + /// + /// Returns `None` if the underlying value type is not a `K`. + /// + /// See also: [`Vc::try_resolve_downcast`]. + pub async fn try_downcast(this: Self) -> Result>, ResolveTypeError> + where + K: Upcast, + K: VcValueTrait + ?Sized + Send, + { + Ok(Vc::try_resolve_downcast(this.node) + .await? + .map(|node| ResolvedVc { node })) + } + + /// Attempts to downcast the given `Vc>` to a `Vc`, where `K` is a value type. + /// + /// Returns `None` if the underlying value type is not a `K`. + /// + /// See also: [`Vc::try_resolve_downcast_type`]. + pub async fn try_downcast_type(this: Self) -> Result>, ResolveTypeError> + where + K: Upcast, + K: VcValueType, + { + Ok(Vc::try_resolve_downcast_type(this.node) + .await? + .map(|node| ResolvedVc { node })) + } +} + impl std::fmt::Debug for ResolvedVc where T: Send, From d0cbe64e8783740ecc1315e04757008f624a0dbd Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Thu, 3 Oct 2024 17:23:04 -0600 Subject: [PATCH 4/4] [i18n] Routing fix (#70761) This fixes a bug where an incorrectly sanitized query parameter would cause an invalid routing condition resulting in the wrong route being served to users. This currently requires some specific edge conditions in order to trigger such as missing i18n configuration, self-hosted, specifically crafted query parameter, and a path-based middleware for authorization. --- packages/next/src/server/base-server.ts | 8 +++++ packages/next/src/server/lib/i18n-provider.ts | 31 +++++++++++++++++++ .../server/lib/router-utils/resolve-routes.ts | 5 +++ 3 files changed, 44 insertions(+) diff --git a/packages/next/src/server/base-server.ts b/packages/next/src/server/base-server.ts index 7c88f4e7620de..b0b71b119cf6b 100644 --- a/packages/next/src/server/base-server.ts +++ b/packages/next/src/server/base-server.ts @@ -1017,6 +1017,14 @@ export default abstract class Server< req.headers['x-forwarded-proto'] ??= isHttps ? 'https' : 'http' req.headers['x-forwarded-for'] ??= originalRequest?.socket?.remoteAddress + // Validate that if i18n isn't configured or the passed parameters are not + // valid it should be removed from the query. + if (!this.i18nProvider?.validateQuery(parsedUrl.query)) { + delete parsedUrl.query.__nextLocale + delete parsedUrl.query.__nextDefaultLocale + delete parsedUrl.query.__nextInferredLocaleFromDefault + } + // This should be done before any normalization of the pathname happens as // it captures the initial URL. this.attachRequestMeta(req, parsedUrl) diff --git a/packages/next/src/server/lib/i18n-provider.ts b/packages/next/src/server/lib/i18n-provider.ts index efdbe4ac12dd4..0a1e7c7503167 100644 --- a/packages/next/src/server/lib/i18n-provider.ts +++ b/packages/next/src/server/lib/i18n-provider.ts @@ -134,6 +134,37 @@ export class I18NProvider { } } + /** + * Validates that the locale is valid. + * + * @param locale The locale to validate. + * @returns `true` if the locale is valid, `false` otherwise. + */ + private validate(locale: string): boolean { + return this.lowerCaseLocales.includes(locale.toLowerCase()) + } + + /** + * Validates that the locales in the query object are valid. + * + * @param query The query object to validate. + * @returns `true` if the locale is valid, `false` otherwise. + */ + public validateQuery(query: NextParsedUrlQuery) { + if (query.__nextLocale && !this.validate(query.__nextLocale)) { + return false + } + + if ( + query.__nextDefaultLocale && + !this.validate(query.__nextDefaultLocale) + ) { + return false + } + + return true + } + /** * Analyzes the pathname for a locale and returns the pathname without it. * diff --git a/packages/next/src/server/lib/router-utils/resolve-routes.ts b/packages/next/src/server/lib/router-utils/resolve-routes.ts index f12a3d2bc3c1e..95ed26105a0be 100644 --- a/packages/next/src/server/lib/router-utils/resolve-routes.ts +++ b/packages/next/src/server/lib/router-utils/resolve-routes.ts @@ -218,6 +218,11 @@ export function getResolveRoutes( parsedUrl.pathname = maybeAddTrailingSlash(parsedUrl.pathname) } } + } else { + // As i18n isn't configured we remove the locale related query params. + delete parsedUrl.query.__nextLocale + delete parsedUrl.query.__nextDefaultLocale + delete parsedUrl.query.__nextInferredLocaleFromDefault } const checkLocaleApi = (pathname: string) => {