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

Apply react-server conditions to middleware #65424

Merged
merged 13 commits into from
May 12, 2024
36 changes: 31 additions & 5 deletions packages/next-swc/crates/next-core/src/next_edge/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ use crate::{
next_import_map::get_next_edge_import_map,
next_server::context::ServerContextType,
next_shared::resolve::{
get_invalid_client_only_resolve_plugin, get_invalid_styled_jsx_resolve_plugin,
ModuleFeatureReportResolvePlugin, NextSharedRuntimeResolvePlugin,
UnsupportedModulesResolvePlugin,
},
Expand Down Expand Up @@ -98,6 +99,35 @@ pub async fn get_edge_resolve_options_context(
get_next_edge_import_map(project_path, ty, next_config, execution_context);

let ty = ty.into_value();
let invalid_client_only_resolve_plugin = get_invalid_client_only_resolve_plugin(project_path);
let invalid_styled_jsx_client_only_resolve_plugin =
get_invalid_styled_jsx_resolve_plugin(project_path);
Copy link
Member

Choose a reason for hiding this comment

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

These two calls could be moved into the branch below to avoid them for the types that don't need them


let mut plugins = match ty {
ServerContextType::Pages { .. }
| ServerContextType::PagesApi { .. }
| ServerContextType::AppSSR { .. } => {
vec![]
}
ServerContextType::AppRSC { .. }
| ServerContextType::AppRoute { .. }
| ServerContextType::PagesData { .. }
| ServerContextType::Middleware { .. }
| ServerContextType::Instrumentation => {
vec![
Vc::upcast(invalid_client_only_resolve_plugin),
Vc::upcast(invalid_styled_jsx_client_only_resolve_plugin),
]
}
};

let base_plugins = vec![
Vc::upcast(ModuleFeatureReportResolvePlugin::new(project_path)),
Vc::upcast(UnsupportedModulesResolvePlugin::new(project_path)),
Vc::upcast(NextSharedRuntimeResolvePlugin::new(project_path)),
];

plugins.extend_from_slice(&base_plugins);

// https://github.com/vercel/next.js/blob/bf52c254973d99fed9d71507a2e818af80b8ade7/packages/next/src/build/webpack-config.ts#L96-L102
let mut custom_conditions = vec![mode.await?.condition().to_string()];
Expand All @@ -119,11 +149,7 @@ pub async fn get_edge_resolve_options_context(
import_map: Some(next_edge_import_map),
module: true,
browser: true,
plugins: vec![
Vc::upcast(ModuleFeatureReportResolvePlugin::new(project_path)),
Vc::upcast(UnsupportedModulesResolvePlugin::new(project_path)),
Vc::upcast(NextSharedRuntimeResolvePlugin::new(project_path)),
],
plugins,
..Default::default()
};

Expand Down
17 changes: 1 addition & 16 deletions packages/next-swc/crates/next-core/src/next_import_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -613,6 +613,7 @@ async fn insert_next_server_special_aliases(
| ServerContextType::PagesApi { .. }
| ServerContextType::AppRSC { .. }
| ServerContextType::AppRoute { .. }
| ServerContextType::Middleware { .. }
| ServerContextType::Instrumentation => {
insert_exact_alias_map(
import_map,
Expand All @@ -637,22 +638,6 @@ async fn insert_next_server_special_aliases(
},
);
}
// Potential the bundle introduced into middleware and api can be poisoned by
// client-only but not being used, so we disabled the `client-only` erroring
// on these layers. `server-only` is still available.
ServerContextType::Middleware => {
insert_exact_alias_map(
import_map,
project_path,
indexmap! {
"server-only" => "next/dist/compiled/server-only/empty".to_string(),
"client-only" => "next/dist/compiled/client-only/index".to_string(),
"next/dist/compiled/server-only" => "next/dist/compiled/server-only/empty".to_string(),
"next/dist/compiled/client-only" => "next/dist/compiled/client-only/index".to_string(),
"next/dist/compiled/client-only/error" => "next/dist/compiled/client-only/index".to_string(),
},
);
}
}

import_map.insert_exact_alias(
Expand Down
12 changes: 5 additions & 7 deletions packages/next-swc/crates/next-core/src/next_server/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ impl ServerContextType {
ServerContextType::AppRSC { .. }
| ServerContextType::AppRoute { .. }
| ServerContextType::PagesApi { .. }
| ServerContextType::Middleware { .. }
)
}
}
Expand Down Expand Up @@ -179,8 +180,8 @@ pub async fn get_server_resolve_options_context(

let mut plugins = match ty {
ServerContextType::Pages { .. }
| ServerContextType::PagesData { .. }
| ServerContextType::PagesApi { .. } => {
| ServerContextType::PagesApi { .. }
| ServerContextType::PagesData { .. } => {
vec![
Vc::upcast(module_feature_report_resolve_plugin),
Vc::upcast(unsupported_modules_resolve_plugin),
Expand Down Expand Up @@ -225,13 +226,13 @@ pub async fn get_server_resolve_options_context(
// means each resolve plugin must be injected only for the context where the
// alias resolves into the error. The alias lives in here: https://github.com/vercel/next.js/blob/0060de1c4905593ea875fa7250d4b5d5ce10897d/packages/next-swc/crates/next-core/src/next_import_map.rs#L534
match ty {
ServerContextType::Pages { .. } => {
ServerContextType::Pages { .. } | ServerContextType::PagesApi { .. } => {
//noop
}
ServerContextType::PagesData { .. }
| ServerContextType::PagesApi { .. }
| ServerContextType::AppRSC { .. }
| ServerContextType::AppRoute { .. }
| ServerContextType::Middleware { .. }
| ServerContextType::Instrumentation => {
plugins.push(Vc::upcast(invalid_client_only_resolve_plugin));
plugins.push(Vc::upcast(invalid_styled_jsx_client_only_resolve_plugin));
Expand All @@ -240,9 +241,6 @@ pub async fn get_server_resolve_options_context(
//[TODO] Build error in this context makes rsc-build-error.ts fail which expects runtime error code
// looks like webpack and turbopack have different order, webpack runs rsc transform first, turbopack triggers resolve plugin first.
}
ServerContextType::Middleware => {
//noop
}
}

let resolve_options_context = ResolveOptionsContext {
Expand Down
4 changes: 2 additions & 2 deletions packages/next/src/build/swc/options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import type {
StyledComponentsConfig,
} from '../../server/config-shared'
import type { ResolvedBaseUrl } from '../load-jsconfig'
import { isWebpackServerOnlyLayer } from '../utils'

const nextDistPath =
/(next[\\/]dist[\\/]shared[\\/]lib)|(next[\\/]dist[\\/]client)|(next[\\/]dist[\\/]pages)/
Expand Down Expand Up @@ -78,8 +79,7 @@ function getBaseSWCOptions({
serverComponents?: boolean
bundleLayer?: WebpackLayerName
}) {
const isReactServerLayer =
bundleLayer === WEBPACK_LAYERS.reactServerComponents
const isReactServerLayer = isWebpackServerOnlyLayer(bundleLayer)
const parserConfig = getParserOptions({ filename, jsConfig })
const paths = jsConfig?.compilerOptions?.paths
const enableDecorators = Boolean(
Expand Down
8 changes: 4 additions & 4 deletions packages/next/src/build/webpack-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1208,7 +1208,7 @@ export default async function getBaseWebpackConfig(
issuerLayer: {
or: [
...WEBPACK_LAYERS.GROUP.serverOnly,
...WEBPACK_LAYERS.GROUP.nonClientServerTarget,
...WEBPACK_LAYERS.GROUP.neutralTarget,
],
},
resolve: {
Expand All @@ -1220,7 +1220,7 @@ export default async function getBaseWebpackConfig(
issuerLayer: {
not: [
...WEBPACK_LAYERS.GROUP.serverOnly,
...WEBPACK_LAYERS.GROUP.nonClientServerTarget,
...WEBPACK_LAYERS.GROUP.neutralTarget,
],
},
resolve: {
Expand Down Expand Up @@ -1252,7 +1252,7 @@ export default async function getBaseWebpackConfig(
issuerLayer: {
not: [
...WEBPACK_LAYERS.GROUP.serverOnly,
...WEBPACK_LAYERS.GROUP.nonClientServerTarget,
...WEBPACK_LAYERS.GROUP.neutralTarget,
],
},
options: {
Expand All @@ -1270,7 +1270,7 @@ export default async function getBaseWebpackConfig(
],
loader: 'empty-loader',
issuerLayer: {
or: WEBPACK_LAYERS.GROUP.nonClientServerTarget,
or: WEBPACK_LAYERS.GROUP.neutralTarget,
},
},
...(hasAppDir
Expand Down
10 changes: 5 additions & 5 deletions packages/next/src/lib/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -168,16 +168,16 @@ const WEBPACK_LAYERS = {
WEBPACK_LAYERS_NAMES.appMetadataRoute,
WEBPACK_LAYERS_NAMES.appRouteHandler,
WEBPACK_LAYERS_NAMES.instrument,
WEBPACK_LAYERS_NAMES.middleware,
],
neutralTarget: [
// pages api
WEBPACK_LAYERS_NAMES.api,
],
clientOnly: [
WEBPACK_LAYERS_NAMES.serverSideRendering,
WEBPACK_LAYERS_NAMES.appPagesBrowser,
],
nonClientServerTarget: [
// middleware and pages api
WEBPACK_LAYERS_NAMES.middleware,
WEBPACK_LAYERS_NAMES.api,
],
app: [
WEBPACK_LAYERS_NAMES.reactServerComponents,
WEBPACK_LAYERS_NAMES.actionBrowser,
Expand Down
4 changes: 4 additions & 0 deletions test/e2e/module-layer/middleware.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
import 'server-only'
import React from 'react'
import { NextResponse } from 'next/server'
// import './lib/mixed-lib'

export function middleware(request) {
if (React.useState) {
throw new Error('React.useState should not be defined in server layer')
}
return NextResponse.next()
}
92 changes: 61 additions & 31 deletions test/e2e/module-layer/module-layer.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import { nextTestSetup } from 'e2e-utils'
import { getRedboxSource, hasRedbox, retry } from 'next-test-utils'

describe('module layer', () => {
const { next, isNextStart } = nextTestSetup({
const { next, isNextStart, isNextDev, isTurbopack } = nextTestSetup({
files: __dirname,
})

Expand Down Expand Up @@ -59,43 +60,72 @@ describe('module layer', () => {
}
}

describe('no server-only in server targets', () => {
const middlewareFile = 'middleware.js'
// const pagesApiFile = 'pages/api/hello.js'
let middlewareContent = ''
// let pagesApiContent = ''
if (isNextDev) {
describe('client packages in middleware', () => {
const middlewareFile = 'middleware.js'
let middlewareContent = ''

beforeAll(async () => {
await next.stop()
afterAll(async () => {
await next.patchFile(middlewareFile, middlewareContent)
})

it('should error when import server packages in middleware', async () => {
const browser = await next.browser('/')

middlewareContent = await next.readFile(middlewareFile)
// pagesApiContent = await next.readFile(pagesApiFile)
middlewareContent = await next.readFile(middlewareFile)

await next.patchFile(
middlewareFile,
middlewareContent
.replace("import 'server-only'", "// import 'server-only'")
.replace("// import './lib/mixed-lib'", "import './lib/mixed-lib'")
)
await next.patchFile(
middlewareFile,
middlewareContent
.replace("import 'server-only'", "// import 'server-only'")
.replace("// import './lib/mixed-lib'", "import './lib/mixed-lib'")
)

// await next.patchFile(
// pagesApiFile,
// pagesApiContent
// .replace("import 'server-only'", "// import 'server-only'")
// .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.`
)
})

await next.start()
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`
)
}
})
})
afterAll(async () => {
await next.patchFile(middlewareFile, middlewareContent)
// await next.patchFile(pagesApiFile, pagesApiContent)

describe('client packages in pages api', () => {
const pagesApiFile = 'pages/api/mixed.js'
let pagesApiContent
beforeAll(async () => {
pagesApiContent = await next.readFile(pagesApiFile)
await next.patchFile(
pagesApiFile,
pagesApiContent.replace(
"// import '../../lib/mixed-lib'",
"import '../../lib/mixed-lib'"
)
)
})
afterAll(async () => {
await next.patchFile(pagesApiFile, pagesApiContent)
})

it('should not error when import client packages in pages/api', async () => {
await retry(async () => {
const { status } = await next.fetch('/api/mixed')
expect(status).toBe(200)
})
})
})
runTests()
})
}

describe('with server-only in server targets', () => {
runTests()
})
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/module-layer/pages/api/hello.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import 'server-only'

export default function handler(req, res) {
return res.send('pages/api/hello.js:')
return res.send('pages/api/hello.js')
}
2 changes: 1 addition & 1 deletion test/e2e/module-layer/pages/api/mixed.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import '../../lib/mixed-lib'
// import '../../lib/mixed-lib'

export default function handler(req, res) {
huozhi marked this conversation as resolved.
Show resolved Hide resolved
return res.send('pages/api/mixed.js:')
Expand Down
Loading