Skip to content

Commit f270834

Browse files
lubieowocelubieowoce
authored
[dynamicIO] fix: do not apply import tracking transform in edge (#79284)
Dynamic import tracking should never be applied to edge runtime code, because: 1. `dynamicIO` prerendering is not supported for edge 2. the implementation of `dynamicIO` prerendering relies on node-specific APIs. I missed this in #74152. This PR fixes a couple things: - We no longer apply the dynamic import tracking to any edge modules - Just to be safe, `trackDynamicImport` and `new CacheSignal` will now throw if it's called from the edge runtime - However, if `track-module-loading` does end up in an edge module, we don't want to error at the top-level when creating `moduleLoadingSignal`, so it is now initialized lazily. This is a bit ugly, but `AppRouteRouteModule` currently pulls all prerendering-related code in at the top level (instead of e.g. a conditional `require()` when prerendering), so it *is* included in edge route handlers, even if unused. - this also required changing `__next_require__`/`__next_chunk_load__` to only track module loading in DIO prerenders, otherwise we'd attempt to create `moduleLoadingSignal` in edge RSC too I also fixed a potential unhandled rejection in `CacheSignal.trackRead`, because `.finally` rejects even if the original error was caught somewhere else. (thanks for finding this one @gnoff!)
1 parent dfcb8cb commit f270834

File tree

14 files changed

+114
-10
lines changed

14 files changed

+114
-10
lines changed

‎crates/next-core/src/next_server/transforms.rs

+6-1
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,12 @@ pub async fn get_next_server_transforms_rules(
178178
ServerContextType::Middleware { .. } | ServerContextType::Instrumentation { .. } => false,
179179
};
180180

181-
if is_app_dir && *next_config.enable_dynamic_io().await? {
181+
if is_app_dir &&
182+
// `dynamicIO` is not supported in the edge runtime.
183+
// (also, the code generated by the dynamic imports transform relies on `CacheSignal`, which uses nodejs-specific APIs)
184+
next_runtime != NextRuntime::Edge &&
185+
*next_config.enable_dynamic_io().await?
186+
{
182187
rules.push(get_next_track_dynamic_imports_transform_rule(mdx_rs));
183188
}
184189

‎packages/next/errors.json

+3-1
Original file line numberDiff line numberDiff line change
@@ -677,5 +677,7 @@
677677
"676": "Invariant: missing internal router-server-methods this is an internal bug",
678678
"677": "`trackDynamicImport` should always receive a promise. Something went wrong in the dynamic imports transform.",
679679
"678": "CacheSignal got more endRead() calls than beginRead() calls",
680-
"679": "A CacheSignal cannot subscribe to itself"
680+
"679": "A CacheSignal cannot subscribe to itself",
681+
"680": "CacheSignal cannot be used in the edge runtime, because `dynamicIO` does not support it.",
682+
"681": "Dynamic imports should not be instrumented in the edge runtime, because `dynamicIO` doesn't support it"
681683
}

‎packages/next/src/build/webpack-config.ts

+1
Original file line numberDiff line numberDiff line change
@@ -534,6 +534,7 @@ export default async function getBaseWebpackConfig(
534534
loader: 'next-swc-loader',
535535
options: {
536536
isServer: isNodeOrEdgeCompilation,
537+
compilerType,
537538
rootDir: dir,
538539
pagesDir,
539540
appDir,

‎packages/next/src/build/webpack/loaders/next-swc-loader.ts

+9-2
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,10 @@ import {
3939
type SwcTransformTelemetryOutput,
4040
} from '../plugins/telemetry-plugin/update-telemetry-loader-context-from-swc'
4141
import type { LoaderContext } from 'webpack'
42+
import {
43+
COMPILER_NAMES,
44+
type CompilerNameValues,
45+
} from '../../../shared/lib/constants'
4246

4347
const maybeExclude = (
4448
excludePath: string,
@@ -57,6 +61,7 @@ const maybeExclude = (
5761
export interface SWCLoaderOptions {
5862
rootDir: string
5963
isServer: boolean
64+
compilerType: CompilerNameValues
6065
pagesDir?: string
6166
appDir?: string
6267
hasReactRefresh: boolean
@@ -214,10 +219,12 @@ async function loaderTransform(
214219
function shouldTrackDynamicImports(loaderOptions: SWCLoaderOptions): boolean {
215220
// we only need to track `import()` 1. in dynamicIO, 2. on the server (RSC and SSR)
216221
// (Note: logic duplicated in crates/next-core/src/next_server/transforms.rs)
217-
const { nextConfig, isServer, bundleLayer } = loaderOptions
222+
const { nextConfig, bundleLayer, compilerType } = loaderOptions
218223
return (
219224
!!nextConfig.experimental?.dynamicIO &&
220-
isServer &&
225+
// NOTE: `server` means nodejs. `dynamicIO` is not supported in the edge runtime, so we want to exclude it.
226+
// (also, the code generated by the dynamic imports transform relies on `CacheSignal`, which uses nodejs-specific APIs)
227+
compilerType === COMPILER_NAMES.server &&
221228
(bundleLayer === WEBPACK_LAYERS.reactServerComponents ||
222229
bundleLayer === WEBPACK_LAYERS.serverSideRendering)
223230
)

‎packages/next/src/server/app-render/app-render.tsx

+18-3
Original file line numberDiff line numberDiff line change
@@ -1203,18 +1203,33 @@ async function renderToHTMLOrFlightImpl(
12031203
// to treat chunk/module loading with similar semantics as cache reads to avoid
12041204
// module loading from causing a prerender to abort too early.
12051205

1206+
const shouldTrackModuleLoading = () => {
1207+
if (!renderOpts.experimental.dynamicIO) {
1208+
return false
1209+
}
1210+
const workUnitStore = workUnitAsyncStorage.getStore()
1211+
return !!(
1212+
workUnitStore &&
1213+
(workUnitStore.type === 'prerender' || workUnitStore.type === 'cache')
1214+
)
1215+
}
1216+
12061217
const __next_require__: typeof instrumented.require = (...args) => {
12071218
const exportsOrPromise = instrumented.require(...args)
1208-
// requiring an async module returns a promise.
1209-
trackPendingImport(exportsOrPromise)
1219+
if (shouldTrackModuleLoading()) {
1220+
// requiring an async module returns a promise.
1221+
trackPendingImport(exportsOrPromise)
1222+
}
12101223
return exportsOrPromise
12111224
}
12121225
// @ts-expect-error
12131226
globalThis.__next_require__ = __next_require__
12141227

12151228
const __next_chunk_load__: typeof instrumented.loadChunk = (...args) => {
12161229
const loadingChunk = instrumented.loadChunk(...args)
1217-
trackPendingChunkLoad(loadingChunk)
1230+
if (shouldTrackModuleLoading()) {
1231+
trackPendingChunkLoad(loadingChunk)
1232+
}
12181233
return loadingChunk
12191234
}
12201235
// @ts-expect-error

‎packages/next/src/server/app-render/cache-signal.ts

+12-1
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,15 @@ export class CacheSignal {
1616

1717
private subscribedSignals: Set<CacheSignal> | null = null
1818

19+
constructor() {
20+
if (process.env.NEXT_RUNTIME === 'edge') {
21+
// we rely on `process.nextTick`, which is not supported in edge
22+
throw new InvariantError(
23+
'CacheSignal cannot be used in the edge runtime, because `dynamicIO` does not support it.'
24+
)
25+
}
26+
}
27+
1928
private noMorePendingCaches() {
2029
if (!this.tickPending) {
2130
this.tickPending = true
@@ -107,7 +116,9 @@ export class CacheSignal {
107116

108117
trackRead<T>(promise: Promise<T>) {
109118
this.beginRead()
110-
promise.finally(this.endRead.bind(this))
119+
// `promise.finally()` still rejects, so don't use it here to avoid unhandled rejections
120+
const onFinally = this.endRead.bind(this)
121+
promise.then(onFinally, onFinally)
111122
return promise
112123
}
113124

‎packages/next/src/server/app-render/module-loading/track-dynamic-import.ts

+6
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,12 @@ import { trackPendingImport } from './track-module-loading.external'
1212
export function trackDynamicImport<TExports extends Record<string, any>>(
1313
modulePromise: Promise<TExports>
1414
): Promise<TExports> {
15+
if (process.env.NEXT_RUNTIME === 'edge') {
16+
throw new InvariantError(
17+
"Dynamic imports should not be instrumented in the edge runtime, because `dynamicIO` doesn't support it"
18+
)
19+
}
20+
1521
if (!isThenable(modulePromise)) {
1622
// We're expecting `import()` to always return a promise. If it's not, something's very wrong.
1723
throw new InvariantError(

‎packages/next/src/server/app-render/module-loading/track-module-loading.instance.ts

+16-2
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,26 @@
11
import { CacheSignal } from '../cache-signal'
22
import { isThenable } from '../../../shared/lib/is-thenable'
33

4-
/** Tracks all in-flight async imports and chunk loads. */
5-
const moduleLoadingSignal = new CacheSignal()
4+
/**
5+
* Tracks all in-flight async imports and chunk loads.
6+
* Initialized lazily, because we don't want this to error in case it gets pulled into an edge runtime module.
7+
*/
8+
let _moduleLoadingSignal: CacheSignal | null
9+
function getModuleLoadingSignal() {
10+
if (!_moduleLoadingSignal) {
11+
_moduleLoadingSignal = new CacheSignal()
12+
}
13+
return _moduleLoadingSignal
14+
}
615

716
export function trackPendingChunkLoad(promise: Promise<unknown>) {
17+
const moduleLoadingSignal = getModuleLoadingSignal()
818
moduleLoadingSignal.trackRead(promise)
919
}
1020

1121
export function trackPendingImport(exportsOrPromise: unknown) {
22+
const moduleLoadingSignal = getModuleLoadingSignal()
23+
1224
// requiring an async module returns a promise.
1325
// if it's sync, there's nothing to track.
1426
if (isThenable(exportsOrPromise)) {
@@ -28,6 +40,8 @@ export function trackPendingImport(exportsOrPromise: unknown) {
2840
* So if we see one, we want to extend the duration of `cacheSignal` at least until the import/chunk-load is done.
2941
*/
3042
export function trackPendingModules(cacheSignal: CacheSignal): void {
43+
const moduleLoadingSignal = getModuleLoadingSignal()
44+
3145
// We can't just use `cacheSignal.trackRead(moduleLoadingSignal.cacheReady())`,
3246
// because we might start and finish multiple batches of module loads while waiting for caches,
3347
// and `moduleLoadingSignal.cacheReady()` would resolve after the first batch.

‎test/e2e/app-dir/dynamic-io-dynamic-imports/bundled/app/not-instrumented/edge-route-handler/messages.ts

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
export default { title: 'hello' }

‎test/e2e/app-dir/dynamic-io-dynamic-imports/bundled/app/not-instrumented/edge-route-handler/route.ts

+9
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
export const runtime = 'edge'
2+
3+
export async function GET(_request: Request) {
4+
// This import should not be instrumented, because edge routes are never prerendered.
5+
// `trackDynamicImport` will throw if it's used in the edge runtime,
6+
// so it's enough to just do an import() here and see if it succeeds.
7+
const messages = (await import('./messages')).default
8+
return new Response(messages.title)
9+
}

‎test/e2e/app-dir/dynamic-io-dynamic-imports/bundled/app/not-instrumented/middleware/page.tsx

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
// This page is only used as a matcher target for the middleware.
2+
3+
export default function Page() {
4+
return 'hello'
5+
}

‎test/e2e/app-dir/dynamic-io-dynamic-imports/bundled/messages.ts

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
export default { title: 'hello' }

‎test/e2e/app-dir/dynamic-io-dynamic-imports/bundled/middleware.ts

+12
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
import type { MiddlewareConfig } from 'next/server'
2+
3+
export default async function middleware() {
4+
// This import should not be instrumented.
5+
// `trackDynamicImport` will throw if it's used in the edge runtime,
6+
// so it's enough to just do an import() here and see if it succeeds.
7+
await import('./messages')
8+
}
9+
10+
export const config: MiddlewareConfig = {
11+
matcher: ['/not-instrumented/middleware'],
12+
}

‎test/e2e/app-dir/dynamic-io-dynamic-imports/dynamic-io-dynamic-imports.test.ts

+15
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ describe('async imports in dynamicIO', () => {
5050
"/inside-render/server/from-node-modules/esm/async-module",
5151
"/inside-render/server/from-node-modules/esm/sync-module",
5252
"/inside-render/server/sync-module",
53+
"/not-instrumented/middleware",
5354
"/outside-of-render/client/async-module",
5455
"/outside-of-render/client/sync-module",
5556
"/outside-of-render/server/async-module",
@@ -154,6 +155,20 @@ describe('async imports in dynamicIO', () => {
154155
})
155156
})
156157
})
158+
159+
describe('are not instrumented in edge', () => {
160+
it('middleware', async () => {
161+
// indirectly tests the behavior of middleware by rendering a page which the middleware matches
162+
await testPage('/not-instrumented/middleware')
163+
})
164+
165+
it('edge route handler', async () => {
166+
const result = await next
167+
.fetch('/not-instrumented/edge-route-handler')
168+
.then((res) => res.text())
169+
expect(result).toBe('hello')
170+
})
171+
})
157172
})
158173

159174
describe('async imports in dynamicIO - external packages', () => {

0 commit comments

Comments
 (0)