Prefer module over main on main fields for app router server compiler (…

…#56960)

Reverts #56766
Reland #56532 

x-ref: https://vercel.slack.com/archives/C04DUD7EB1B/p1697576563771229?thread_ts=1697146531.305779&cid=C04DUD7EB1B

This change was breaking package which was actually a dual package but marked as esm module, and when the package is ESM but import a CJS dependency without fully spepcified file path it will fail to resolve. The way is to either remove the esmodule type in package.json or change the import to full path import with extension for the package

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' ? ';' : ':')
@@ -936,7 +925,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,
     }),
@@ -1044,7 +1034,6 @@ export default async function getBaseWebpackConfig(
     config,
     optOutBundlingPackageRegex,
     dir,
-    hasAppDir,
   })
 
   const shouldIncludeExternalDirs =
@@ -1615,6 +1604,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
@@ -1759,6 +1749,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"
+}