Revert "Revert "Prefer module over main on main fields for app router server compiler" (#56766)"

This reverts commit 476af24.

Author: huozhi

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<string, string>
   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

packages/next/src/build/webpack-config-rules/resolve.ts

@@ -0,0 +1,40 @@
+import {
+  COMPILER_NAMES,
+  type CompilerNameValues,
+} from '../../shared/lib/constants'
+
+// exports.<conditionName>
+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]
+}

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 = <T>(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.<conditionName>
-const edgeConditionNames = [
-  'edge-light',
-  'worker',
-  // inherits the default conditions
-  '...',
-]
-
-// packageJson.<mainField>
-const mainFieldsPerCompiler: Record<CompilerNameValues, string[]> = {
-  [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),
+                    },
                   },
                 ]
               : []),

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)
             },

test/e2e/app-dir/app-external/app-external.test.ts

@@ -39,14 +39,15 @@ 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)
         expect(result).toContain('Server: index.default')
         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 () => {

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}`}
       <br />
       <Client />
+      <br />
+      <div id="main-field">{`Server module field: ${serverFieldName}`}</div>
     </div>
   )
 }

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}`}
       <br />
       <Client />
+      <p id="optout-react-version">
+        {`opt-out-react-version: ${getReactVersion()}`}
+      </p>
     </div>
   )
 }

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"
   }
 }

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
+}

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"
   }
 }

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
+}

test/e2e/app-dir/app-external/node_modules_bak/server-module-field/index.cjs

@@ -0,0 +1 @@
+exports.name = 'server-module-field:main'

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'

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"
+}