Skip to content

Commit

Permalink
Revert "Prefer module over main on main fields for app router server …
Browse files Browse the repository at this point in the history
…compiler" (#56766)

This was causing some issues with our deployments.

[slack x-ref](https://vercel.slack.com/archives/C04DUD7EB1B/p1697146531305779)
  • Loading branch information
ztanner authored Oct 12, 2023
1 parent 961fd01 commit 476af24
Show file tree
Hide file tree
Showing 14 changed files with 47 additions and 99 deletions.
11 changes: 10 additions & 1 deletion packages/next/src/build/handle-externals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ export async function resolveExternal(
context: string,
request: string,
isEsmRequested: boolean,
hasAppDir: boolean,
getResolve: (
options: any
) => (
Expand All @@ -65,7 +66,11 @@ 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
Expand Down Expand Up @@ -130,10 +135,12 @@ 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'
Expand Down Expand Up @@ -286,6 +293,7 @@ export function makeExternalHandler({
context,
request,
isEsmRequested,
hasAppDir,
getResolve,
isLocal ? resolveNextExternal : undefined
)
Expand Down Expand Up @@ -345,6 +353,7 @@ export function makeExternalHandler({
config.experimental.esmExternals,
context,
pkg + '/package.json',
hasAppDir,
isEsmRequested,
getResolve,
isLocal ? resolveNextExternal : undefined
Expand Down
40 changes: 0 additions & 40 deletions packages/next/src/build/webpack-config-rules/resolve.ts

This file was deleted.

27 changes: 17 additions & 10 deletions packages/next/src/build/webpack-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,6 @@ 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 = {
Expand Down Expand Up @@ -108,6 +104,21 @@ 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' ? ';' : ':')
Expand Down Expand Up @@ -920,8 +931,7 @@ export default async function getBaseWebpackConfig(
},
}
: undefined),
// default main fields use pages dir ones, and customize app router ones in loaders.
mainFields: getMainField('pages', compilerType),
mainFields: mainFieldsPerCompiler[compilerType],
...(isEdgeServer && {
conditionNames: edgeConditionNames,
}),
Expand Down Expand Up @@ -1029,6 +1039,7 @@ export default async function getBaseWebpackConfig(
config,
optOutBundlingPackageRegex,
dir,
hasAppDir,
})

const shouldIncludeExternalDirs =
Expand Down Expand Up @@ -1599,7 +1610,6 @@ 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
Expand Down Expand Up @@ -1744,9 +1754,6 @@ export default async function getBaseWebpackConfig(
],
exclude: [codeCondition.exclude],
use: swcLoaderForClientLayer,
resolve: {
mainFields: getMainField('app', compilerType),
},
},
]
: []),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -743,6 +743,7 @@ export class TraceEntryPointsPlugin implements webpack.WebpackPluginInstance {
context,
request,
isEsmRequested,
!!this.appDirEnabled,
(options) => (_: string, resRequest: string) => {
return getResolve(options)(parent, resRequest, job)
},
Expand Down
38 changes: 19 additions & 19 deletions test/e2e/app-dir/app-external/app-external.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,14 @@ createNextDescribe(
buildCommand: 'yarn build',
skipDeployment: true,
},
({ next }) => {
({ next, isNextDev }) => {
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')
})
})

Expand Down Expand Up @@ -92,23 +91,24 @@ createNextDescribe(
})

it('should resolve 3rd party package exports based on the react-server condition', async () => {
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')
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')
})
})

it('should correctly collect global css imports and mark them as side effects', async () => {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import v from 'conditional-exports'
import v1 from 'conditional-exports/subpath'
import { name as serverFieldName } from 'server-module-field'

import Client from './client'

Expand All @@ -12,8 +11,6 @@ export default function Page() {
{`Server subpath: ${v1}`}
<br />
<Client />
<br />
<div id="main-field">{`Server module field: ${serverFieldName}`}</div>
</div>
)
}
4 changes: 0 additions & 4 deletions test/e2e/app-dir/app-external/app/react-server/optout/page.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import v from 'conditional-exports-optout'
import v1 from 'conditional-exports-optout/subpath'
import { getReactVersion } from 'conditional-exports-optout/react'

import Client from './client'

Expand All @@ -12,9 +11,6 @@ export default function Page() {
{`Server subpath: ${v1}`}
<br />
<Client />
<p id="optout-react-version">
{`opt-out-react-version: ${getReactVersion()}`}
</p>
</div>
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,6 @@
"react-server": "./subpath.server.js",
"default": "./subpath.js"
},
"./react": {
"import": "./react.js"
},
"./package.json": "./package.json"
}
}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,6 @@
"react-server": "./subpath.server.js",
"default": "./subpath.js"
},
"./react": {
"import": "./react.js"
},
"./package.json": "./package.json"
}
}

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

0 comments on commit 476af24

Please sign in to comment.