From bf89bee37d8b1bbb0c16dc65957c4ad01299f067 Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Tue, 7 May 2024 21:14:58 +0200 Subject: [PATCH] Support esm externals in app router (#65041) ### What Support `esmExternals` working in app router ### Why `esmExternals` was disabled for app router that most of the packages are picking up the CJS bundles for externals. This PR enables to resolve the ESM bundle for external packages. We have two issues discovered while enabling the flag, any esm external packages will fail in client page SSR and server action. We fixed them by changing the below bundling logics: * When a client page having a async dependency, we can await the page during in rendering * When a server action having a async dependency, we changed the server action entry creation with webpack along with the server client entry creation together, then webpack can handle the modules async propagation properly. Fixes #60756 Closes NEXT-2435 Closes NEXT-2472 Closes NEXT-3225 --------- Co-authored-by: Tobias Koppers --- packages/next/src/build/handle-externals.ts | 93 ++++-------- .../plugins/flight-client-entry-plugin.ts | 135 +++++++++--------- .../app-render/create-component-tree.tsx | 2 +- .../app-dir/app-external/app-external.test.ts | 9 +- test/e2e/esm-externals/esm-externals.test.ts | 21 +-- 5 files changed, 114 insertions(+), 146 deletions(-) diff --git a/packages/next/src/build/handle-externals.ts b/packages/next/src/build/handle-externals.ts index 6db179278f33..bb26df1b8a4e 100644 --- a/packages/next/src/build/handle-externals.ts +++ b/packages/next/src/build/handle-externals.ts @@ -1,5 +1,6 @@ -import { WEBPACK_LAYERS } from '../lib/constants' import type { WebpackLayerName } from '../lib/constants' +import type { NextConfigComplete } from '../server/config-shared' +import type { ResolveOptions } from 'webpack' import { defaultOverrides } from '../server/require-hook' import { BARREL_OPTIMIZATION_PREFIX } from '../shared/lib/constants' import path from '../shared/lib/isomorphic/path' @@ -10,7 +11,6 @@ import { NODE_RESOLVE_OPTIONS, } from './webpack-config' import { isWebpackAppLayer, isWebpackServerOnlyLayer } from './utils' -import type { NextConfigComplete } from '../server/config-shared' import { normalizePathSep } from '../shared/lib/page-path/normalize-path-sep' const reactPackagesRegex = /^(react|react-dom|react-server-dom-webpack)($|\/)/ @@ -25,15 +25,6 @@ const externalPattern = new RegExp( const nodeModulesRegex = /node_modules[/\\].*\.[mc]?js$/ -function containsImportInPackages( - request: string, - packages: string[] -): boolean { - return packages.some( - (pkg) => request === pkg || request.startsWith(pkg + '/') - ) -} - export function isResourceInPackages( resource: string, packageNames?: string[], @@ -57,9 +48,9 @@ export async function resolveExternal( context: string, request: string, isEsmRequested: boolean, - optOutBundlingPackages: string[], + _optOutBundlingPackages: string[], getResolve: ( - options: any + options: ResolveOptions ) => ( resolveContext: string, resolveRequest: string @@ -78,19 +69,12 @@ export async function resolveExternal( let isEsm: boolean = false const preferEsmOptions = - esmExternals && - isEsmRequested && - // For package that marked as externals that should be not bundled, - // we don't resolve them as ESM since it could be resolved as async module, - // such as `import(external package)` in the bundle, valued as a `Promise`. - !containsImportInPackages(request, optOutBundlingPackages) - ? [true, false] - : [false] + esmExternals && isEsmRequested ? [true, false] : [false] for (const preferEsm of preferEsmOptions) { - const resolve = getResolve( - preferEsm ? esmResolveOptions : nodeResolveOptions - ) + const resolveOptions = preferEsm ? esmResolveOptions : nodeResolveOptions + + const resolve = getResolve(resolveOptions) // Resolve the import with the webpack provided context, this // ensures we're resolving the correct version when multiple @@ -273,23 +257,6 @@ export function makeExternalHandler({ return resolveNextExternal(request) } - // Early return if the request needs to be bundled, such as in the client layer. - // Treat react packages and next internals as external for SSR layer, - // also map react to builtin ones with require-hook. - // Otherwise keep continue the process to resolve the externals. - if (layer === WEBPACK_LAYERS.serverSideRendering) { - const isRelative = request.startsWith('.') - const fullRequest = isRelative - ? normalizePathSep(path.join(context, request)) - : request - - // Check if it's opt out bundling package first - if (containsImportInPackages(fullRequest, optOutBundlingPackages)) { - return fullRequest - } - return resolveNextExternal(fullRequest) - } - // TODO-APP: Let's avoid this resolve call as much as possible, and eventually get rid of it. const resolveResult = await resolveExternal( dir, @@ -320,6 +287,13 @@ export function makeExternalHandler({ return } + const isOptOutBundling = optOutBundlingPackageRegex.test(res) + // Apply bundling rules to all app layers. + // Since handleExternals only handle the server layers, we don't need to exclude client here + if (!isOptOutBundling && isAppLayer) { + return + } + // ESM externals can only be imported (and not required). // Make an exception in loose mode. if (!isEsmRequested && isEsm && !looseEsmExternals && !isLocal) { @@ -370,13 +344,11 @@ export function makeExternalHandler({ const resolvedBundlingOptOutRes = resolveBundlingOptOutPackages({ resolvedRes: res, - optOutBundlingPackageRegex, config, resolvedExternalPackageDirs, - isEsm, isAppLayer, - layer, externalType, + isOptOutBundling, request, }) if (resolvedBundlingOptOutRes) { @@ -390,41 +362,32 @@ export function makeExternalHandler({ function resolveBundlingOptOutPackages({ resolvedRes, - optOutBundlingPackageRegex, config, resolvedExternalPackageDirs, - isEsm, isAppLayer, - layer, externalType, + isOptOutBundling, request, }: { resolvedRes: string - optOutBundlingPackageRegex: RegExp config: NextConfigComplete resolvedExternalPackageDirs: Map - isEsm: boolean isAppLayer: boolean - layer: WebpackLayerName | null externalType: string + isOptOutBundling: boolean request: string }) { - const shouldBeBundled = - isResourceInPackages( - resolvedRes, - config.transpilePackages, - resolvedExternalPackageDirs - ) || - (isEsm && isAppLayer) || - (!isAppLayer && config.bundlePagesRouterDependencies) - if (nodeModulesRegex.test(resolvedRes)) { - const isOptOutBundling = optOutBundlingPackageRegex.test(resolvedRes) - if (isWebpackServerOnlyLayer(layer)) { - if (isOptOutBundling) { - return `${externalType} ${request}` // Externalize if opted out - } - } else if (!shouldBeBundled || isOptOutBundling) { + const shouldBundlePages = + !isAppLayer && config.bundlePagesRouterDependencies && !isOptOutBundling + const shouldBeBundled = + shouldBundlePages || + isResourceInPackages( + resolvedRes, + config.transpilePackages, + resolvedExternalPackageDirs + ) + if (!shouldBeBundled) { return `${externalType} ${request}` // Externalize if not bundled or opted out } } 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 932c5356c3e0..cdb25cce5dae 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 @@ -430,75 +430,6 @@ export class FlightClientEntryPlugin { ) } - compilation.hooks.finishModules.tapPromise(PLUGIN_NAME, async () => { - const addedClientActionEntryList: Promise[] = [] - const actionMapsPerClientEntry: Record> = {} - - // We need to create extra action entries that are created from the - // client layer. - // Start from each entry's created SSR dependency from our previous step. - for (const [name, ssrEntryDependencies] of Object.entries( - createdSSRDependenciesForEntry - )) { - // Collect from all entries, e.g. layout.js, page.js, loading.js, ... - // add aggregate them. - const actionEntryImports = this.collectClientActionsFromDependencies({ - compilation, - dependencies: ssrEntryDependencies, - }) - - if (actionEntryImports.size > 0) { - if (!actionMapsPerClientEntry[name]) { - actionMapsPerClientEntry[name] = new Map() - } - actionMapsPerClientEntry[name] = new Map([ - ...actionMapsPerClientEntry[name], - ...actionEntryImports, - ]) - } - } - - for (const [name, actionEntryImports] of Object.entries( - actionMapsPerClientEntry - )) { - // If an action method is already created in the server layer, we don't - // need to create it again in the action layer. - // This is to avoid duplicate action instances and make sure the module - // state is shared. - let remainingClientImportedActions = false - const remainingActionEntryImports = new Map() - for (const [dep, actionNames] of actionEntryImports) { - const remainingActionNames = [] - for (const actionName of actionNames) { - const id = name + '@' + dep + '@' + actionName - if (!createdActions.has(id)) { - remainingActionNames.push(actionName) - } - } - if (remainingActionNames.length > 0) { - remainingActionEntryImports.set(dep, remainingActionNames) - remainingClientImportedActions = true - } - } - - if (remainingClientImportedActions) { - addedClientActionEntryList.push( - this.injectActionEntry({ - compiler, - compilation, - actions: remainingActionEntryImports, - entryName: name, - bundlePath: name, - fromClient: true, - }) - ) - } - } - - await Promise.all(addedClientActionEntryList) - return - }) - // Invalidate in development to trigger recompilation const invalidator = getInvalidator(compiler.outputPath) // Check if any of the entry injections need an invalidation @@ -521,6 +452,72 @@ export class FlightClientEntryPlugin { // Wait for action entries to be added. await Promise.all(addActionEntryList) + + const addedClientActionEntryList: Promise[] = [] + const actionMapsPerClientEntry: Record> = {} + + // We need to create extra action entries that are created from the + // client layer. + // Start from each entry's created SSR dependency from our previous step. + for (const [name, ssrEntryDependencies] of Object.entries( + createdSSRDependenciesForEntry + )) { + // Collect from all entries, e.g. layout.js, page.js, loading.js, ... + // add aggregate them. + const actionEntryImports = this.collectClientActionsFromDependencies({ + compilation, + dependencies: ssrEntryDependencies, + }) + + if (actionEntryImports.size > 0) { + if (!actionMapsPerClientEntry[name]) { + actionMapsPerClientEntry[name] = new Map() + } + actionMapsPerClientEntry[name] = new Map([ + ...actionMapsPerClientEntry[name], + ...actionEntryImports, + ]) + } + } + + for (const [name, actionEntryImports] of Object.entries( + actionMapsPerClientEntry + )) { + // If an action method is already created in the server layer, we don't + // need to create it again in the action layer. + // This is to avoid duplicate action instances and make sure the module + // state is shared. + let remainingClientImportedActions = false + const remainingActionEntryImports = new Map() + for (const [dep, actionNames] of actionEntryImports) { + const remainingActionNames = [] + for (const actionName of actionNames) { + const id = name + '@' + dep + '@' + actionName + if (!createdActions.has(id)) { + remainingActionNames.push(actionName) + } + } + if (remainingActionNames.length > 0) { + remainingActionEntryImports.set(dep, remainingActionNames) + remainingClientImportedActions = true + } + } + + if (remainingClientImportedActions) { + addedClientActionEntryList.push( + this.injectActionEntry({ + compiler, + compilation, + actions: remainingActionEntryImports, + entryName: name, + bundlePath: name, + fromClient: true, + }) + ) + } + } + + await Promise.all(addedClientActionEntryList) } collectClientActionsFromDependencies({ diff --git a/packages/next/src/server/app-render/create-component-tree.tsx b/packages/next/src/server/app-render/create-component-tree.tsx index e7a076f0baec..acee5b460119 100644 --- a/packages/next/src/server/app-render/create-component-tree.tsx +++ b/packages/next/src/server/app-render/create-component-tree.tsx @@ -265,7 +265,7 @@ async function createComponentTreeInternal({ } const LayoutOrPage: React.ComponentType | undefined = layoutOrPageMod - ? interopDefault(layoutOrPageMod) + ? await interopDefault(layoutOrPageMod) : undefined /** diff --git a/test/e2e/app-dir/app-external/app-external.test.ts b/test/e2e/app-dir/app-external/app-external.test.ts index 733e14baf455..0df01e0f7e0f 100644 --- a/test/e2e/app-dir/app-external/app-external.test.ts +++ b/test/e2e/app-dir/app-external/app-external.test.ts @@ -16,7 +16,7 @@ async function resolveStreamResponse(response: any, onData?: any) { } describe('app dir - external dependency', () => { - const { next, skipped } = nextTestSetup({ + const { next, skipped, isTurbopack } = nextTestSetup({ files: __dirname, dependencies: { swr: 'latest', @@ -279,14 +279,17 @@ describe('app dir - external dependency', () => { }) describe('server actions', () => { - it('should not prefer to resolve esm over cjs for bundling optout packages', async () => { + it('should prefer to resolve esm over cjs for bundling optout packages', async () => { const browser = await next.browser('/optout/action') expect(await browser.elementByCss('#dual-pkg-outout p').text()).toBe('') browser.elementByCss('#dual-pkg-outout button').click() await check(async () => { const text = await browser.elementByCss('#dual-pkg-outout p').text() - expect(text).toBe('dual-pkg-optout:cjs') + // TODO: enable esm externals for app router in turbopack for actions + expect(text).toBe( + isTurbopack ? 'dual-pkg-optout:cjs' : 'dual-pkg-optout:mjs' + ) return 'success' }, /success/) }) diff --git a/test/e2e/esm-externals/esm-externals.test.ts b/test/e2e/esm-externals/esm-externals.test.ts index 4832bb6b5881..e4ebc9e40cd5 100644 --- a/test/e2e/esm-externals/esm-externals.test.ts +++ b/test/e2e/esm-externals/esm-externals.test.ts @@ -13,13 +13,15 @@ describe('esm-externals', () => { const urls = ['/static', '/ssr', '/ssg'] for (const url of urls) { - // TODO fix webpack behavior + // For invalid esm packages that have "import" pointing to a non-esm-flagged module + // webpack is using the CJS version instead but Turbopack is opting out of + // externalizing and bundling the non-esm-flagged module. const expectedHtml = isTurbopack ? /Hello World\+World\+World\+World\+World\+World/ : url === '/static' ? /Hello World\+World\+Alternative\+World\+World\+World/ : /Hello World\+World\+Alternative\+World\+World\+Alternative/ - // TODO fix webpack behavior + // On client side, webpack always bundlings so it uses the non-esm-flagged module too. const expectedText = isTurbopack || url === '/static' ? /Hello World\+World\+World\+World\+World\+World/ @@ -40,18 +42,21 @@ describe('esm-externals', () => { // App dir { - // TODO App Dir doesn't use esmExternals: true correctly for webpack and Turbopack + // TODO App Dir doesn't use esmExternals: true correctly for Turbopack // so we only verify that the page doesn't crash, but ignore the actual content - // const expectedHtml = /Hello World\+World\+World/ - const expectedHtml = /Hello Wrong\+Wrong\+Alternative/ - // const expectedText = /Hello World\+World\+World/ + // const expectedHtml = isTurbopack ? /Hello World\+World\+World/ : /Hello World\+World\+Alternative/ + const expectedHtml = isTurbopack + ? /Hello Wrong\+Wrong\+Alternative/ + : /Hello World\+World\+Alternative/ const urls = ['/server', '/client'] for (const url of urls) { const expectedText = - url === '/server' + url !== '/server' + ? /Hello World\+World\+World/ + : isTurbopack ? /Hello Wrong\+Wrong\+Alternative/ - : /Hello World\+World\+World/ + : /Hello World\+World\+Alternative/ it(`should return the correct SSR HTML for ${url}`, async () => { const res = await next.fetch(url) const html = await res.text()