Skip to content

Commit

Permalink
Fix server output bundling packages module resolving (#59369)
Browse files Browse the repository at this point in the history
  • Loading branch information
huozhi authored Dec 7, 2023
1 parent 42ec6c8 commit 2874bc0
Show file tree
Hide file tree
Showing 10 changed files with 80 additions and 4 deletions.
16 changes: 14 additions & 2 deletions packages/next/src/build/handle-externals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ export async function resolveExternal(
context: string,
request: string,
isEsmRequested: boolean,
optOutBundlingPackages: string[],
getResolve: (
options: any
) => (
Expand All @@ -66,8 +67,15 @@ export async function resolveExternal(
let res: string | null = null
let isEsm: boolean = false

let preferEsmOptions =
esmExternals && isEsmRequested ? [true, false] : [false]
const preferEsmOptions =
esmExternals &&
isEsmRequested &&
// For package that marked as externals that should be not bundled,
// we don't resolve them as ESM since it could be resolved as async module,
// such as `import(external package)` in the bundle, valued as a `Promise`.
!optOutBundlingPackages.some((optOut) => request.startsWith(optOut))
? [true, false]
: [false]

for (const preferEsm of preferEsmOptions) {
const resolve = getResolve(
Expand Down Expand Up @@ -131,10 +139,12 @@ export async function resolveExternal(

export function makeExternalHandler({
config,
optOutBundlingPackages,
optOutBundlingPackageRegex,
dir,
}: {
config: NextConfigComplete
optOutBundlingPackages: string[]
optOutBundlingPackageRegex: RegExp
dir: string
}) {
Expand Down Expand Up @@ -289,6 +299,7 @@ export function makeExternalHandler({
context,
request,
isEsmRequested,
optOutBundlingPackages,
getResolve,
isLocal ? resolveNextExternal : undefined
)
Expand Down Expand Up @@ -349,6 +360,7 @@ export function makeExternalHandler({
context,
pkg + '/package.json',
isEsmRequested,
optOutBundlingPackages,
getResolve,
isLocal ? resolveNextExternal : undefined
)
Expand Down
4 changes: 4 additions & 0 deletions packages/next/src/build/webpack-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -727,9 +727,11 @@ export default async function getBaseWebpackConfig(

const crossOrigin = config.crossOrigin

// For original request, such as `package name`
const optOutBundlingPackages = EXTERNAL_PACKAGES.concat(
...(config.experimental.serverComponentsExternalPackages || [])
)
// For resolved request, such as `absolute path/package name/foo/bar.js`
const optOutBundlingPackageRegex = new RegExp(
`[/\\\\]node_modules[/\\\\](${optOutBundlingPackages
.map((p) => p.replace(/\//g, '[/\\\\]'))
Expand All @@ -738,6 +740,7 @@ export default async function getBaseWebpackConfig(

const handleExternals = makeExternalHandler({
config,
optOutBundlingPackages,
optOutBundlingPackageRegex,
dir,
})
Expand Down Expand Up @@ -1662,6 +1665,7 @@ export default async function getBaseWebpackConfig(
outputFileTracingRoot: config.experimental.outputFileTracingRoot,
appDirEnabled: hasAppDir,
turbotrace: config.experimental.turbotrace,
optOutBundlingPackages,
traceIgnores: config.experimental.outputFileTracingIgnores || [],
}
),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ export class TraceEntryPointsPlugin implements webpack.WebpackPluginInstance {
private rootDir: string
private appDir: string | undefined
private pagesDir: string | undefined
private optOutBundlingPackages: string[]
private appDirEnabled?: boolean
private tracingRoot: string
private entryTraces: Map<string, Set<string>>
Expand All @@ -144,6 +145,7 @@ export class TraceEntryPointsPlugin implements webpack.WebpackPluginInstance {
rootDir,
appDir,
pagesDir,
optOutBundlingPackages,
appDirEnabled,
traceIgnores,
esmExternals,
Expand All @@ -153,6 +155,7 @@ export class TraceEntryPointsPlugin implements webpack.WebpackPluginInstance {
rootDir: string
appDir: string | undefined
pagesDir: string | undefined
optOutBundlingPackages: string[]
appDirEnabled?: boolean
traceIgnores?: string[]
outputFileTracingRoot?: string
Expand All @@ -168,6 +171,7 @@ export class TraceEntryPointsPlugin implements webpack.WebpackPluginInstance {
this.traceIgnores = traceIgnores || []
this.tracingRoot = outputFileTracingRoot || rootDir
this.turbotrace = turbotrace
this.optOutBundlingPackages = optOutBundlingPackages
}

// Here we output all traced assets and webpack chunks to a
Expand Down Expand Up @@ -743,6 +747,7 @@ export class TraceEntryPointsPlugin implements webpack.WebpackPluginInstance {
context,
request,
isEsmRequested,
this.optOutBundlingPackages,
(options) => (_: string, resRequest: string) => {
return getResolve(options)(parent, resRequest, job)
},
Expand Down
19 changes: 18 additions & 1 deletion test/e2e/app-dir/app-external/app-external.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { createNextDescribe } from 'e2e-utils'
import { shouldRunTurboDevTest } from '../../../lib/next-test-utils'
import { check, shouldRunTurboDevTest } from 'next-test-utils'

async function resolveStreamResponse(response: any, onData?: any) {
let result = ''
Expand Down Expand Up @@ -250,5 +250,22 @@ createNextDescribe(
const html = await next.render('/async-storage')
expect(html).toContain('success')
})

it('should not prefer to resolve esm over cjs for bundling optout packages', async () => {
const browser = await next.browser('/optout/action')
expect(await browser.elementByCss('#dual-pkg-outout p').text()).toBe('')

browser.elementByCss('#dual-pkg-outout button').click()
await check(async () => {
const text = await browser.elementByCss('#dual-pkg-outout p').text()
if (process.env.TURBOPACK) {
// The prefer esm won't effect turbopack resolving
expect(text).toBe('dual-pkg-optout:mjs')
} else {
expect(text).toBe('dual-pkg-optout:cjs')
}
return 'success'
}, /success/)
})
}
)
7 changes: 7 additions & 0 deletions test/e2e/app-dir/app-external/app/optout/action/actions.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
'use server'

import { value as dualPkgOptoutValue } from 'dual-pkg-optout'

export async function getDualOptoutValue() {
return dualPkgOptoutValue
}
20 changes: 20 additions & 0 deletions test/e2e/app-dir/app-external/app/optout/action/page.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
'use client'

import { useState } from 'react'
import { getDualOptoutValue } from './actions'

export default function Page() {
const [optoutDisplayValue, setOptoutDisplayValue] = useState('')
return (
<div id="dual-pkg-outout">
<p>{optoutDisplayValue}</p>
<button
onClick={async () => {
setOptoutDisplayValue(await getDualOptoutValue())
}}
>
dual-pkg-optout
</button>
</div>
)
}
5 changes: 4 additions & 1 deletion test/e2e/app-dir/app-external/next.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ module.exports = {
reactStrictMode: true,
transpilePackages: ['untranspiled-module', 'css', 'font'],
experimental: {
serverComponentsExternalPackages: ['conditional-exports-optout'],
serverComponentsExternalPackages: [
'conditional-exports-optout',
'dual-pkg-optout',
],
},
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
exports.value = 'dual-pkg-optout:cjs'
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const value = 'dual-pkg-optout:mjs'
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"exports": {
"import": "./index.mjs",
"require": "./index.cjs"
}
}

0 comments on commit 2874bc0

Please sign in to comment.