From 6814ff147a416a437c541fd2a27e69e1794af700 Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Thu, 12 Oct 2023 18:10:55 +0200 Subject: [PATCH] Prefer module over main on main fields for app router server compiler (#56532) This change is to pick the esm assets for RSC server layer and server rendering side, this is for production and development perf purpose, also to let them apply with the ESM related optimization such as tree-shaking, barrel files optimizations, etc. We found a few packages that can't be optimized easily in bundling because we're using "main" field so the packages are not able to be tree-shaked, ending up with large bundle in the dist. This will change a lot for the bundling improvements as some packages are only having "main" and "module" field. So switching from CJS to ESM means better bundling, more optimization, and faster code. #56501 was a precondition for this, as previously the bundling strategy was applied to some library but triggered the invalid hooks erroring. ### Other Monior Change Previously we'll prefer to resolve CJS as there're 2 versions of react, using CJS assets will help let them pass by require-hook to use canary react for app router bundling assets. But now we changed the approach to bundling nextjs runtime and react packages. Now we dropped the condition that prefered to resolve CJS exports for externals, since if you're putting them in `serverComponentsExternalPackages`, they're not using the built-in react, so could potentially having trouble if any dependency is using react but excluded in bundles. So far we didn't see any report to this. Closes NEXT-1286 --- packages/next/src/build/handle-externals.ts | 11 +---- .../src/build/webpack-config-rules/resolve.ts | 40 +++++++++++++++++++ packages/next/src/build/webpack-config.ts | 27 +++++-------- .../plugins/next-trace-entrypoints-plugin.ts | 1 - .../app-dir/app-external/app-external.test.ts | 38 +++++++++--------- .../react-server/3rd-party-package/page.js | 3 ++ .../app/react-server/optout/page.js | 4 ++ .../conditional-exports-optout/package.json | 3 ++ .../conditional-exports-optout/react.js | 5 +++ .../conditional-exports/package.json | 3 ++ .../conditional-exports/react.js | 5 +++ .../server-module-field/index.cjs | 1 + .../server-module-field/index.esm.js | 1 + .../server-module-field/package.json | 4 ++ 14 files changed, 99 insertions(+), 47 deletions(-) create mode 100644 packages/next/src/build/webpack-config-rules/resolve.ts create mode 100644 test/e2e/app-dir/app-external/node_modules_bak/conditional-exports-optout/react.js create mode 100644 test/e2e/app-dir/app-external/node_modules_bak/conditional-exports/react.js create mode 100644 test/e2e/app-dir/app-external/node_modules_bak/server-module-field/index.cjs create mode 100644 test/e2e/app-dir/app-external/node_modules_bak/server-module-field/index.esm.js create mode 100644 test/e2e/app-dir/app-external/node_modules_bak/server-module-field/package.json diff --git a/packages/next/src/build/handle-externals.ts b/packages/next/src/build/handle-externals.ts index 2e0eb816b1495..853ab2e3c68ab 100644 --- a/packages/next/src/build/handle-externals.ts +++ b/packages/next/src/build/handle-externals.ts @@ -44,7 +44,6 @@ export async function resolveExternal( context: string, request: string, isEsmRequested: boolean, - hasAppDir: boolean, getResolve: ( options: any ) => ( @@ -66,11 +65,7 @@ export async function resolveExternal( let preferEsmOptions = esmExternals && isEsmRequested ? [true, false] : [false] - // Disable esm resolving for app/ and pages/ so for esm package using under pages/ - // won't load react through esm loader - if (hasAppDir) { - preferEsmOptions = [false] - } + for (const preferEsm of preferEsmOptions) { const resolve = getResolve( preferEsm ? esmResolveOptions : nodeResolveOptions @@ -135,12 +130,10 @@ export function makeExternalHandler({ config, optOutBundlingPackageRegex, dir, - hasAppDir, }: { config: NextConfigComplete optOutBundlingPackageRegex: RegExp dir: string - hasAppDir: boolean }) { let resolvedExternalPackageDirs: Map const looseEsmExternals = config.experimental?.esmExternals === 'loose' @@ -293,7 +286,6 @@ export function makeExternalHandler({ context, request, isEsmRequested, - hasAppDir, getResolve, isLocal ? resolveNextExternal : undefined ) @@ -353,7 +345,6 @@ export function makeExternalHandler({ config.experimental.esmExternals, context, pkg + '/package.json', - hasAppDir, isEsmRequested, getResolve, isLocal ? resolveNextExternal : undefined diff --git a/packages/next/src/build/webpack-config-rules/resolve.ts b/packages/next/src/build/webpack-config-rules/resolve.ts new file mode 100644 index 0000000000000..f50f6c92ee629 --- /dev/null +++ b/packages/next/src/build/webpack-config-rules/resolve.ts @@ -0,0 +1,40 @@ +import { + COMPILER_NAMES, + type CompilerNameValues, +} from '../../shared/lib/constants' + +// exports. +export const edgeConditionNames = [ + 'edge-light', + 'worker', + // inherits the default conditions + '...', +] + +const mainFieldsPerCompiler: Record< + CompilerNameValues | 'app-router-server', + string[] +> = { + // For default case, prefer CJS over ESM on server side. e.g. pages dir SSR + [COMPILER_NAMES.server]: ['main', 'module'], + [COMPILER_NAMES.client]: ['browser', 'module', 'main'], + [COMPILER_NAMES.edgeServer]: edgeConditionNames, + // For app router since everything is bundled, prefer ESM over CJS + 'app-router-server': ['module', 'main'], +} + +export function getMainField( + pageType: 'app' | 'pages', + compilerType: CompilerNameValues +) { + if (compilerType === COMPILER_NAMES.edgeServer) { + return edgeConditionNames + } else if (compilerType === COMPILER_NAMES.client) { + return mainFieldsPerCompiler[COMPILER_NAMES.client] + } + + // Prefer module fields over main fields for isomorphic packages on server layer + return pageType === 'app' + ? mainFieldsPerCompiler['app-router-server'] + : mainFieldsPerCompiler[COMPILER_NAMES.server] +} diff --git a/packages/next/src/build/webpack-config.ts b/packages/next/src/build/webpack-config.ts index 1664d4377787c..a20cf97a1e97f 100644 --- a/packages/next/src/build/webpack-config.ts +++ b/packages/next/src/build/webpack-config.ts @@ -74,6 +74,10 @@ import { needsExperimentalReact } from '../lib/needs-experimental-react' import { getDefineEnvPlugin } from './webpack/plugins/define-env-plugin' import type { SWCLoaderOptions } from './webpack/loaders/next-swc-loader' import { isResourceInPackages, makeExternalHandler } from './handle-externals' +import { + getMainField, + edgeConditionNames, +} from './webpack-config-rules/resolve' type ExcludesFalse = (x: T | false) => x is T type ClientEntries = { @@ -104,21 +108,6 @@ const babelIncludeRegexes: RegExp[] = [ const asyncStoragesRegex = /next[\\/]dist[\\/](esm[\\/])?client[\\/]components[\\/](static-generation-async-storage|action-async-storage|request-async-storage)/ -// exports. -const edgeConditionNames = [ - 'edge-light', - 'worker', - // inherits the default conditions - '...', -] - -// packageJson. -const mainFieldsPerCompiler: Record = { - [COMPILER_NAMES.server]: ['main', 'module'], - [COMPILER_NAMES.client]: ['browser', 'module', 'main'], - [COMPILER_NAMES.edgeServer]: edgeConditionNames, -} - // Support for NODE_PATH const nodePathList = (process.env.NODE_PATH || '') .split(process.platform === 'win32' ? ';' : ':') @@ -931,7 +920,8 @@ export default async function getBaseWebpackConfig( }, } : undefined), - mainFields: mainFieldsPerCompiler[compilerType], + // default main fields use pages dir ones, and customize app router ones in loaders. + mainFields: getMainField('pages', compilerType), ...(isEdgeServer && { conditionNames: edgeConditionNames, }), @@ -1039,7 +1029,6 @@ export default async function getBaseWebpackConfig( config, optOutBundlingPackageRegex, dir, - hasAppDir, }) const shouldIncludeExternalDirs = @@ -1610,6 +1599,7 @@ export default async function getBaseWebpackConfig( ], }, resolve: { + mainFields: getMainField('app', compilerType), conditionNames: reactServerCondition, // If missing the alias override here, the default alias will be used which aliases // react to the direct file path, not the package name. In that case the condition @@ -1754,6 +1744,9 @@ export default async function getBaseWebpackConfig( ], exclude: [codeCondition.exclude], use: swcLoaderForClientLayer, + resolve: { + mainFields: getMainField('app', compilerType), + }, }, ] : []), diff --git a/packages/next/src/build/webpack/plugins/next-trace-entrypoints-plugin.ts b/packages/next/src/build/webpack/plugins/next-trace-entrypoints-plugin.ts index 1f61d3520d29d..5ea193152a283 100644 --- a/packages/next/src/build/webpack/plugins/next-trace-entrypoints-plugin.ts +++ b/packages/next/src/build/webpack/plugins/next-trace-entrypoints-plugin.ts @@ -743,7 +743,6 @@ export class TraceEntryPointsPlugin implements webpack.WebpackPluginInstance { context, request, isEsmRequested, - !!this.appDirEnabled, (options) => (_: string, resRequest: string) => { return getResolve(options)(parent, resRequest, job) }, 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 38592f5ba1b47..b4e66cb205966 100644 --- a/test/e2e/app-dir/app-external/app-external.test.ts +++ b/test/e2e/app-dir/app-external/app-external.test.ts @@ -39,7 +39,7 @@ createNextDescribe( buildCommand: 'yarn build', skipDeployment: true, }, - ({ next, isNextDev }) => { + ({ next }) => { it('should be able to opt-out 3rd party packages being bundled in server components', async () => { await next.fetch('/react-server/optout').then(async (response) => { const result = await resolveStreamResponse(response) @@ -47,6 +47,7 @@ createNextDescribe( expect(result).toContain('Server subpath: subpath.default') expect(result).toContain('Client: index.default') expect(result).toContain('Client subpath: subpath.default') + expect(result).toContain('opt-out-react-version: 18.2.0') }) }) @@ -91,24 +92,23 @@ createNextDescribe( }) it('should resolve 3rd party package exports based on the react-server condition', async () => { - await next - .fetch('/react-server/3rd-party-package') - .then(async (response) => { - const result = await resolveStreamResponse(response) - - // Package should be resolved based on the react-server condition, - // as well as package's internal & external dependencies. - expect(result).toContain( - 'Server: index.react-server:react.subset:dep.server' - ) - expect(result).toContain( - 'Client: index.default:react.full:dep.default' - ) - - // Subpath exports should be resolved based on the condition too. - expect(result).toContain('Server subpath: subpath.react-server') - expect(result).toContain('Client subpath: subpath.default') - }) + const $ = await next.render$('/react-server/3rd-party-package') + + const result = $('body').text() + + // Package should be resolved based on the react-server condition, + // as well as package's internal & external dependencies. + expect(result).toContain( + 'Server: index.react-server:react.subset:dep.server' + ) + expect(result).toContain('Client: index.default:react.full:dep.default') + + // Subpath exports should be resolved based on the condition too. + expect(result).toContain('Server subpath: subpath.react-server') + expect(result).toContain('Client subpath: subpath.default') + + // Prefer `module` field for isomorphic packages. + expect($('#main-field').text()).toContain('server-module-field:module') }) it('should correctly collect global css imports and mark them as side effects', async () => { diff --git a/test/e2e/app-dir/app-external/app/react-server/3rd-party-package/page.js b/test/e2e/app-dir/app-external/app/react-server/3rd-party-package/page.js index 33141e12f7685..92e9f01672faa 100644 --- a/test/e2e/app-dir/app-external/app/react-server/3rd-party-package/page.js +++ b/test/e2e/app-dir/app-external/app/react-server/3rd-party-package/page.js @@ -1,5 +1,6 @@ import v from 'conditional-exports' import v1 from 'conditional-exports/subpath' +import { name as serverFieldName } from 'server-module-field' import Client from './client' @@ -11,6 +12,8 @@ export default function Page() { {`Server subpath: ${v1}`}
+
+
{`Server module field: ${serverFieldName}`}
) } diff --git a/test/e2e/app-dir/app-external/app/react-server/optout/page.js b/test/e2e/app-dir/app-external/app/react-server/optout/page.js index fc7bd55ab86c3..45ba1ccff0358 100644 --- a/test/e2e/app-dir/app-external/app/react-server/optout/page.js +++ b/test/e2e/app-dir/app-external/app/react-server/optout/page.js @@ -1,5 +1,6 @@ import v from 'conditional-exports-optout' import v1 from 'conditional-exports-optout/subpath' +import { getReactVersion } from 'conditional-exports-optout/react' import Client from './client' @@ -11,6 +12,9 @@ export default function Page() { {`Server subpath: ${v1}`}
+

+ {`opt-out-react-version: ${getReactVersion()}`} +

) } diff --git a/test/e2e/app-dir/app-external/node_modules_bak/conditional-exports-optout/package.json b/test/e2e/app-dir/app-external/node_modules_bak/conditional-exports-optout/package.json index fe1b70d109b2a..3355c20aad28a 100644 --- a/test/e2e/app-dir/app-external/node_modules_bak/conditional-exports-optout/package.json +++ b/test/e2e/app-dir/app-external/node_modules_bak/conditional-exports-optout/package.json @@ -10,6 +10,9 @@ "react-server": "./subpath.server.js", "default": "./subpath.js" }, + "./react": { + "import": "./react.js" + }, "./package.json": "./package.json" } } diff --git a/test/e2e/app-dir/app-external/node_modules_bak/conditional-exports-optout/react.js b/test/e2e/app-dir/app-external/node_modules_bak/conditional-exports-optout/react.js new file mode 100644 index 0000000000000..4f2c2283ed693 --- /dev/null +++ b/test/e2e/app-dir/app-external/node_modules_bak/conditional-exports-optout/react.js @@ -0,0 +1,5 @@ +import React from 'react' + +export function getReactVersion() { + return React.version +} diff --git a/test/e2e/app-dir/app-external/node_modules_bak/conditional-exports/package.json b/test/e2e/app-dir/app-external/node_modules_bak/conditional-exports/package.json index b51ade2e7acfe..06e09e177ae16 100644 --- a/test/e2e/app-dir/app-external/node_modules_bak/conditional-exports/package.json +++ b/test/e2e/app-dir/app-external/node_modules_bak/conditional-exports/package.json @@ -16,6 +16,9 @@ "react-server": "./subpath.server.js", "default": "./subpath.js" }, + "./react": { + "import": "./react.js" + }, "./package.json": "./package.json" } } diff --git a/test/e2e/app-dir/app-external/node_modules_bak/conditional-exports/react.js b/test/e2e/app-dir/app-external/node_modules_bak/conditional-exports/react.js new file mode 100644 index 0000000000000..4f2c2283ed693 --- /dev/null +++ b/test/e2e/app-dir/app-external/node_modules_bak/conditional-exports/react.js @@ -0,0 +1,5 @@ +import React from 'react' + +export function getReactVersion() { + return React.version +} diff --git a/test/e2e/app-dir/app-external/node_modules_bak/server-module-field/index.cjs b/test/e2e/app-dir/app-external/node_modules_bak/server-module-field/index.cjs new file mode 100644 index 0000000000000..bead07e159aa3 --- /dev/null +++ b/test/e2e/app-dir/app-external/node_modules_bak/server-module-field/index.cjs @@ -0,0 +1 @@ +exports.name = 'server-module-field:main' diff --git a/test/e2e/app-dir/app-external/node_modules_bak/server-module-field/index.esm.js b/test/e2e/app-dir/app-external/node_modules_bak/server-module-field/index.esm.js new file mode 100644 index 0000000000000..02218634f7d07 --- /dev/null +++ b/test/e2e/app-dir/app-external/node_modules_bak/server-module-field/index.esm.js @@ -0,0 +1 @@ +export const name = 'server-module-field:module' diff --git a/test/e2e/app-dir/app-external/node_modules_bak/server-module-field/package.json b/test/e2e/app-dir/app-external/node_modules_bak/server-module-field/package.json new file mode 100644 index 0000000000000..d6cb0ed97cb3d --- /dev/null +++ b/test/e2e/app-dir/app-external/node_modules_bak/server-module-field/package.json @@ -0,0 +1,4 @@ +{ + "main": "./index.cjs", + "module": "./index.esm.js" +}