Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix server output bundling packages module resolving #59369

Merged
merged 3 commits into from
Dec 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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"
}
}