From 8108de42916236e7ab9358a0420d51bd18735f7d Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Fri, 13 Oct 2023 12:14:42 -0400 Subject: [PATCH] Revert "Revert "Prefer module over main on main fields for app router server compiler" (#56766)" This reverts commit 476af246289d90c8f5672ffc3588bf64250331f3. --- 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" +}