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

Reland bunling webpack middleware changes (#66049) #66052

Merged
merged 9 commits into from
May 28, 2024
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
11 changes: 7 additions & 4 deletions packages/next/src/build/entries.ts
Original file line number Diff line number Diff line change
Expand Up @@ -812,11 +812,14 @@ export function finalizeEntrypoint({
}
}
case COMPILER_NAMES.edgeServer: {
const layer = isApi
? WEBPACK_LAYERS.api
: isMiddlewareFilename(name) || isInstrumentation
? WEBPACK_LAYERS.middleware
: undefined

return {
layer:
isMiddlewareFilename(name) || isApi || isInstrumentation
? WEBPACK_LAYERS.middleware
: undefined,
layer,
library: { name: ['_ENTRIES', `middleware_[name]`], type: 'assign' },
runtime: EDGE_RUNTIME_WEBPACK,
asyncChunks: false,
Expand Down
4 changes: 2 additions & 2 deletions packages/next/src/build/handle-externals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
NODE_ESM_RESOLVE_OPTIONS,
NODE_RESOLVE_OPTIONS,
} from './webpack-config'
import { isWebpackAppLayer, isWebpackServerOnlyLayer } from './utils'
import { isWebpackBundledLayer, isWebpackServerOnlyLayer } from './utils'
import { normalizePathSep } from '../shared/lib/page-path/normalize-path-sep'
const reactPackagesRegex = /^(react|react-dom|react-server-dom-webpack)($|\/)/

Expand Down Expand Up @@ -174,7 +174,7 @@ export function makeExternalHandler({
return `commonjs next/dist/lib/import-next-warning`
}

const isAppLayer = isWebpackAppLayer(layer)
const isAppLayer = isWebpackBundledLayer(layer)

// Relative requires don't need custom resolution, because they
// are relative to requests we've already resolved here.
Expand Down
13 changes: 11 additions & 2 deletions packages/next/src/build/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2256,6 +2256,15 @@ export function getSupportedBrowsers(
return MODERN_BROWSERSLIST_TARGET
}

// Use next/dist/compiled/react packages instead of installed react
export function isWebpackBuiltinReactLayer(
layer: WebpackLayerName | null | undefined
): boolean {
return Boolean(
layer && WEBPACK_LAYERS.GROUP.builtinReact.includes(layer as any)
)
}

export function isWebpackServerOnlyLayer(
layer: WebpackLayerName | null | undefined
): boolean {
Expand All @@ -2278,8 +2287,8 @@ export function isWebpackDefaultLayer(
return layer === null || layer === undefined
}

export function isWebpackAppLayer(
export function isWebpackBundledLayer(
layer: WebpackLayerName | null | undefined
): boolean {
return Boolean(layer && WEBPACK_LAYERS.GROUP.app.includes(layer as any))
return Boolean(layer && WEBPACK_LAYERS.GROUP.bundled.includes(layer as any))
}
33 changes: 19 additions & 14 deletions packages/next/src/build/webpack-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ import { escapeStringRegexp } from '../shared/lib/escape-regexp'
import { WEBPACK_LAYERS, WEBPACK_RESOURCE_QUERIES } from '../lib/constants'
import type { WebpackLayerName } from '../lib/constants'
import {
isWebpackAppLayer,
isWebpackBuiltinReactLayer,
isWebpackBundledLayer,
isWebpackClientOnlyLayer,
isWebpackDefaultLayer,
isWebpackServerOnlyLayer,
Expand Down Expand Up @@ -409,8 +410,7 @@ export default async function getBaseWebpackConfig(
loggedIgnoredCompilerOptions = true
}

const shouldIncludeExternalDirs =
config.experimental.externalDir || !!config.transpilePackages
const shouldIncludeExternalDirs = config.experimental.externalDir
const codeCondition = {
test: { or: [/\.(tsx|ts|js|cjs|mjs|jsx)$/, /__barrel_optimize__/] },
...(shouldIncludeExternalDirs
Expand Down Expand Up @@ -543,7 +543,7 @@ export default async function getBaseWebpackConfig(
// This will cause some performance overhead but
// acceptable as Babel will not be recommended.
getSwcLoader({
serverComponents: false,
serverComponents: true,
bundleLayer: WEBPACK_LAYERS.middleware,
}),
babelLoader,
Expand Down Expand Up @@ -592,13 +592,12 @@ export default async function getBaseWebpackConfig(
// Loader for API routes needs to be differently configured as it shouldn't
// have RSC transpiler enabled, so syntax checks such as invalid imports won't
// be performed.
const apiRoutesLayerLoaders =
hasAppDir && useSWCLoader
? getSwcLoader({
serverComponents: false,
bundleLayer: WEBPACK_LAYERS.api,
})
: defaultLoaders.babel
const apiRoutesLayerLoaders = useSWCLoader
? getSwcLoader({
serverComponents: false,
bundleLayer: WEBPACK_LAYERS.api,
})
: defaultLoaders.babel

const pageExtensions = config.pageExtensions

Expand Down Expand Up @@ -1304,7 +1303,7 @@ export default async function getBaseWebpackConfig(
test: /next[\\/]dist[\\/](esm[\\/])?server[\\/]future[\\/]route-modules[\\/]app-page[\\/]module/,
},
{
issuerLayer: isWebpackAppLayer,
issuerLayer: isWebpackBundledLayer,
resolve: {
alias: createNextApiEsmAliases(),
},
Expand All @@ -1326,7 +1325,7 @@ export default async function getBaseWebpackConfig(
...(hasAppDir && !isClient
? [
{
issuerLayer: isWebpackServerOnlyLayer,
issuerLayer: isWebpackBuiltinReactLayer,
test: {
// Resolve it if it is a source code file, and it has NOT been
// opted out of bundling.
Expand Down Expand Up @@ -1388,7 +1387,7 @@ export default async function getBaseWebpackConfig(
// Alias react for switching between default set and share subset.
oneOf: [
{
issuerLayer: isWebpackServerOnlyLayer,
issuerLayer: isWebpackBuiltinReactLayer,
test: {
// Resolve it if it is a source code file, and it has NOT been
// opted out of bundling.
Expand Down Expand Up @@ -1469,11 +1468,17 @@ export default async function getBaseWebpackConfig(
test: codeCondition.test,
issuerLayer: WEBPACK_LAYERS.middleware,
use: middlewareLayerLoaders,
resolve: {
conditionNames: reactServerCondition,
},
},
{
test: codeCondition.test,
issuerLayer: WEBPACK_LAYERS.instrument,
use: instrumentLayerLoaders,
resolve: {
conditionNames: reactServerCondition,
},
},
...(hasAppDir
? [
Expand Down
11 changes: 8 additions & 3 deletions packages/next/src/build/webpack/plugins/middleware-plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,10 @@ import type { Telemetry } from '../../../telemetry/storage'
import { traceGlobals } from '../../../trace/shared'
import { EVENT_BUILD_FEATURE_USAGE } from '../../../telemetry/events'
import { normalizeAppPath } from '../../../shared/lib/router/utils/app-paths'
import { INSTRUMENTATION_HOOK_FILENAME } from '../../../lib/constants'
import {
INSTRUMENTATION_HOOK_FILENAME,
WEBPACK_LAYERS,
} from '../../../lib/constants'
import type { CustomRoutes } from '../../../lib/load-custom-routes'
import { isInterceptionRouteRewrite } from '../../../lib/generate-interception-routes-rewrites'
import { getDynamicCodeEvaluationError } from './wellknown-errors-plugin/parse-dynamic-code-evaluation-error'
Expand Down Expand Up @@ -272,7 +275,8 @@ function buildWebpackError({
}

function isInMiddlewareLayer(parser: webpack.javascript.JavascriptParser) {
return parser.state.module?.layer === 'middleware'
const layer = parser.state.module?.layer
return layer === WEBPACK_LAYERS.middleware || layer === WEBPACK_LAYERS.api
}

function isNodeJsModule(moduleName: string) {
Expand Down Expand Up @@ -849,7 +853,8 @@ export async function handleWebpackExternalForEdgeRuntime({
getResolve: () => any
}) {
if (
contextInfo.issuerLayer === 'middleware' &&
(contextInfo.issuerLayer === WEBPACK_LAYERS.middleware ||
contextInfo.issuerLayer === WEBPACK_LAYERS.api) &&
isNodeJsModule(request) &&
!supportedEdgePolyfills.has(request)
) {
Expand Down
7 changes: 6 additions & 1 deletion packages/next/src/lib/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,11 @@ export type WebpackLayerName =
const WEBPACK_LAYERS = {
...WEBPACK_LAYERS_NAMES,
GROUP: {
builtinReact: [
WEBPACK_LAYERS_NAMES.reactServerComponents,
WEBPACK_LAYERS_NAMES.actionBrowser,
WEBPACK_LAYERS_NAMES.appMetadataRoute,
],
serverOnly: [
WEBPACK_LAYERS_NAMES.reactServerComponents,
WEBPACK_LAYERS_NAMES.actionBrowser,
Expand All @@ -174,7 +179,7 @@ const WEBPACK_LAYERS = {
WEBPACK_LAYERS_NAMES.serverSideRendering,
WEBPACK_LAYERS_NAMES.appPagesBrowser,
],
app: [
bundled: [
WEBPACK_LAYERS_NAMES.reactServerComponents,
WEBPACK_LAYERS_NAMES.actionBrowser,
WEBPACK_LAYERS_NAMES.appMetadataRoute,
Expand Down
13 changes: 11 additions & 2 deletions test/e2e/module-layer/middleware.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,20 @@
import 'server-only'
import React from 'react'
import * as React from 'react'
import { NextResponse } from 'next/server'
// import './lib/mixed-lib'

export function middleware(request) {
if (React.useState) {
// To avoid webpack ESM exports checking warning
const ReactObject = Object(React)
if (ReactObject.useState) {
throw new Error('React.useState should not be defined in server layer')
}

if (request.nextUrl.pathname === '/react-version') {
return Response.json({
React: Object.keys(ReactObject),
})
}

return NextResponse.next()
}
60 changes: 45 additions & 15 deletions test/e2e/module-layer/module-layer.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ import { getRedboxSource, hasRedbox, retry } from 'next-test-utils'
describe('module layer', () => {
const { next, isNextStart, isNextDev, isTurbopack } = nextTestSetup({
files: __dirname,
dependencies: {
react: '19.0.0-rc-915b914b3a-20240515',
'react-dom': '19.0.0-rc-915b914b3a-20240515',
},
})

function runTests() {
Expand All @@ -18,8 +22,10 @@ describe('module layer', () => {
'/app/route',
'/app/route-edge',
// pages/api
'/api/hello',
'/api/hello-edge',
'/api/default',
'/api/default-edge',
'/api/server-only',
'/api/server-only-edge',
'/api/mixed',
]

Expand All @@ -30,6 +36,35 @@ describe('module layer', () => {
})
}

it('should render installed react-server condition for middleware', async () => {
const json = await next.fetch('/react-version').then((res) => res.json())
expect(json.React).toContain('version') // basic react-server export
expect(json.React).not.toContain('useEffect') // no client api export
})

// This is for backward compatibility, don't change react usage in existing pages/api
it('should contain client react exports for pages api', async () => {
async function verifyReactExports(route, isEdge) {
const json = await next.fetch(route).then((res) => res.json())
// contain all react-server and default condition exports
expect(json.React).toContain('version')
expect(json.React).toContain('useEffect')

// contain react-dom-server default condition exports
expect(json.ReactDomServer).toContain('version')
expect(json.ReactDomServer).toContain('renderToString')
expect(json.ReactDomServer).toContain('renderToStaticMarkup')
expect(json.ReactDomServer).toContain(
isEdge ? 'renderToReadableStream' : 'renderToPipeableStream'
)
Comment on lines +50 to +59
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd just do a full expect(json.ReactDomServer).toEqual() so that we immediately learn when new stuff (that may not be supposed to) appears here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this because don't want to cover the properties like react internal, but only the rest ones that are widly used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't have minded that one. Might even help highlight bundling issues.

}

await verifyReactExports('/api/default', false)
await verifyReactExports('/api/default-edge', true)
await verifyReactExports('/api/server-only', false)
await verifyReactExports('/api/server-only-edge', true)
})

if (isNextStart) {
it('should log the build info properly', async () => {
const cliOutput = next.cliOutput
Expand All @@ -40,7 +75,8 @@ describe('module layer', () => {
)
expect(functionsManifest.functions).toContainKeys([
'/app/route-edge',
'/api/hello-edge',
'/api/default-edge',
'/api/server-only-edge',
'/app/client-edge',
'/app/server-edge',
])
Expand All @@ -52,9 +88,10 @@ describe('module layer', () => {
)
expect(middlewareManifest.middleware).toBeTruthy()
expect(pagesManifest).toContainKeys([
'/api/hello-edge',
'/api/default-edge',
'/pages-ssr',
'/api/hello',
'/api/default',
'/api/server-only',
])
})
}
Expand All @@ -81,22 +118,15 @@ describe('module layer', () => {
.replace("// import './lib/mixed-lib'", "import './lib/mixed-lib'")
)

const existingCliOutputLength = next.cliOutput.length
await retry(async () => {
expect(await hasRedbox(browser)).toBe(true)
const source = await getRedboxSource(browser)
expect(source).toContain(
`'client-only' cannot be imported from a Server Component module. It should only be used from a Client Component.`
isTurbopack
? `'client-only' cannot be imported from a Server Component module. It should only be used from a Client Component.`
: `You're importing a component that imports client-only. It only works in a Client Component but none of its parents are marked with "use client"`
)
})

if (!isTurbopack) {
const newCliOutput = next.cliOutput.slice(existingCliOutputLength)
expect(newCliOutput).toContain('./middleware.js')
expect(newCliOutput).toContain(
`'client-only' cannot be imported from a Server Component module. It should only be used from a Client Component`
)
}
})
})
}
Expand Down
11 changes: 11 additions & 0 deletions test/e2e/module-layer/pages/api/default-edge.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import * as ReactDomServer from 'react-dom/server'
import * as React from 'react'

export default async (_req) => {
return Response.json({
React: Object.keys(Object(React)),
ReactDomServer: Object.keys(Object(ReactDomServer)),
})
}

export const runtime = 'edge'
9 changes: 9 additions & 0 deletions test/e2e/module-layer/pages/api/default.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import * as ReactDomServer from 'react-dom/server'
import * as React from 'react'

export default async (_req, res) => {
return res.json({
React: Object.keys(Object(React)),
ReactDomServer: Object.keys(Object(ReactDomServer)),
})
}
7 changes: 0 additions & 7 deletions test/e2e/module-layer/pages/api/hello-edge.js

This file was deleted.

5 changes: 0 additions & 5 deletions test/e2e/module-layer/pages/api/hello.js

This file was deleted.

12 changes: 12 additions & 0 deletions test/e2e/module-layer/pages/api/server-only-edge.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import 'server-only'
import * as ReactDomServer from 'react-dom/server'
import * as React from 'react'

export default async (_req) => {
return Response.json({
React: Object.keys(Object(React)),
ReactDomServer: Object.keys(Object(ReactDomServer)),
})
}

export const runtime = 'edge'
Loading
Loading