From 7fe01bb63911b027f29ce2e0e908b0c44af42433 Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Mon, 18 Sep 2023 21:59:54 +0200 Subject: [PATCH] Disable client-only for middleware and pages api layer (#55541) We need to disable the default treat `middleware` and `pages/api` as server-only, unless users explictly import "server-only" to poison it. This will avoid the case that when a library is mixing "client-only" API and shared components API in one bundle, and the shared API is used in middleware or `pages/api` that might cause error. See the test case added. Follow up for #55394 --- packages/next/src/build/webpack-config.ts | 39 ++++- .../src/build/webpack/loaders/empty-loader.ts | 4 + packages/next/src/lib/constants.ts | 7 +- test/e2e/module-layer/index.test.ts | 134 ++++++++++++------ test/e2e/module-layer/lib/mixed-lib/client.js | 3 + test/e2e/module-layer/lib/mixed-lib/index.js | 3 + test/e2e/module-layer/lib/mixed-lib/shared.js | 1 + test/e2e/module-layer/middleware.js | 3 +- test/e2e/module-layer/pages/api/hello-edge.js | 2 +- test/e2e/module-layer/pages/api/hello.js | 2 +- test/e2e/module-layer/pages/api/mixed.js | 5 + 11 files changed, 142 insertions(+), 61 deletions(-) create mode 100644 packages/next/src/build/webpack/loaders/empty-loader.ts create mode 100644 test/e2e/module-layer/lib/mixed-lib/client.js create mode 100644 test/e2e/module-layer/lib/mixed-lib/index.js create mode 100644 test/e2e/module-layer/lib/mixed-lib/shared.js create mode 100644 test/e2e/module-layer/pages/api/mixed.js diff --git a/packages/next/src/build/webpack-config.ts b/packages/next/src/build/webpack-config.ts index 0dbd31c89cbeb..a171641c46923 100644 --- a/packages/next/src/build/webpack-config.ts +++ b/packages/next/src/build/webpack-config.ts @@ -926,13 +926,13 @@ export default async function getBaseWebpackConfig( : [] const swcLoaderForMiddlewareLayer = useSWCLoader - ? getSwcLoader({ hasServerComponents: false, bundleTarget: 'server' }) + ? getSwcLoader({ hasServerComponents: false, bundleTarget: 'default' }) : // When using Babel, we will have to use SWC to do the optimization // for middleware to tree shake the unused default optimized imports like "next/server". // This will cause some performance overhead but // acceptable as Babel will not be recommended. [ - getSwcLoader({ hasServerComponents: false, bundleTarget: 'server' }), + getSwcLoader({ hasServerComponents: false, bundleTarget: 'default' }), getBabelLoader(), ] @@ -980,7 +980,7 @@ export default async function getBaseWebpackConfig( loader: 'next-swc-loader', options: { ...getSwcLoader().options, - bundleTarget: 'server', + bundleTarget: 'default', hasServerComponents: false, }, } @@ -1965,6 +1965,7 @@ export default async function getBaseWebpackConfig( 'next-flight-action-entry-loader', 'next-flight-client-module-loader', 'noop-loader', + 'empty-loader', 'next-middleware-loader', 'next-edge-function-loader', 'next-edge-app-route-loader', @@ -2041,7 +2042,10 @@ export default async function getBaseWebpackConfig( // Alias server-only and client-only to proper exports based on bundling layers { issuerLayer: { - or: WEBPACK_LAYERS.GROUP.serverTarget, + or: [ + ...WEBPACK_LAYERS.GROUP.server, + ...WEBPACK_LAYERS.GROUP.nonClientServerTarget, + ], }, resolve: { // Error on client-only but allow server-only @@ -2050,12 +2054,17 @@ export default async function getBaseWebpackConfig( 'client-only$': 'next/dist/compiled/client-only/error', 'next/dist/compiled/server-only$': 'next/dist/compiled/server-only/empty', + 'next/dist/compiled/client-only$': + 'next/dist/compiled/client-only/error', }, }, }, { issuerLayer: { - not: WEBPACK_LAYERS.GROUP.serverTarget, + not: [ + ...WEBPACK_LAYERS.GROUP.server, + ...WEBPACK_LAYERS.GROUP.nonClientServerTarget, + ], }, resolve: { // Error on server-only but allow client-only @@ -2077,7 +2086,7 @@ export default async function getBaseWebpackConfig( ], loader: 'next-invalid-import-error-loader', issuerLayer: { - or: WEBPACK_LAYERS.GROUP.serverTarget, + or: WEBPACK_LAYERS.GROUP.server, }, options: { message: @@ -2091,13 +2100,29 @@ export default async function getBaseWebpackConfig( ], loader: 'next-invalid-import-error-loader', issuerLayer: { - not: WEBPACK_LAYERS.GROUP.serverTarget, + not: [ + ...WEBPACK_LAYERS.GROUP.server, + ...WEBPACK_LAYERS.GROUP.nonClientServerTarget, + ], }, options: { message: "'server-only' cannot be imported from a Client Component module. It should only be used from a Server Component.", }, }, + // 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. + { + test: [ + /^client-only$/, + /next[\\/]dist[\\/]compiled[\\/]client-only[\\/]error/, + ], + loader: 'empty-loader', + issuerLayer: { + or: WEBPACK_LAYERS.GROUP.nonClientServerTarget, + }, + }, ...(hasAppDir ? [ { diff --git a/packages/next/src/build/webpack/loaders/empty-loader.ts b/packages/next/src/build/webpack/loaders/empty-loader.ts new file mode 100644 index 0000000000000..7b283791e2b2e --- /dev/null +++ b/packages/next/src/build/webpack/loaders/empty-loader.ts @@ -0,0 +1,4 @@ +import type { webpack } from 'next/dist/compiled/webpack/webpack' + +const EmptyLoader: webpack.LoaderDefinitionFunction = () => 'export default {}' +export default EmptyLoader diff --git a/packages/next/src/lib/constants.ts b/packages/next/src/lib/constants.ts index e3f8f07838c4c..0ec3d8c07f894 100644 --- a/packages/next/src/lib/constants.ts +++ b/packages/next/src/lib/constants.ts @@ -152,12 +152,7 @@ const WEBPACK_LAYERS = { WEBPACK_LAYERS_NAMES.appMetadataRoute, WEBPACK_LAYERS_NAMES.appRouteHandler, ], - serverTarget: [ - // all GROUP.server - WEBPACK_LAYERS_NAMES.reactServerComponents, - WEBPACK_LAYERS_NAMES.actionBrowser, - WEBPACK_LAYERS_NAMES.appMetadataRoute, - WEBPACK_LAYERS_NAMES.appRouteHandler, + nonClientServerTarget: [ // plus middleware and pages api WEBPACK_LAYERS_NAMES.middleware, WEBPACK_LAYERS_NAMES.api, diff --git a/test/e2e/module-layer/index.test.ts b/test/e2e/module-layer/index.test.ts index 6e4359b6a21aa..4aef0e35246fe 100644 --- a/test/e2e/module-layer/index.test.ts +++ b/test/e2e/module-layer/index.test.ts @@ -6,55 +6,99 @@ createNextDescribe( files: __dirname, }, ({ next, isNextStart }) => { - it('should render routes marked with restriction marks without errors', async () => { - const routes = [ - // app client components pages - '/app/client', - '/app/client-edge', - // app sever components pages - '/app/server', - '/app/server-edge', - // app routes - '/app/route', - '/app/route-edge', - // pages/api - '/api/hello', - '/api/hello-edge', - ] - - for (const route of routes) { - const { status } = await next.fetch(route) - expect([route, status]).toEqual([route, 200]) - } - }) - - if (isNextStart) { - it('should log the build info properly', async () => { - const cliOutput = next.cliOutput - expect(cliOutput).toContain('Middleware') - - const functionsManifest = JSON.parse( - await next.readFile('.next/server/functions-config-manifest.json') - ) - expect(functionsManifest.functions).toContainKeys([ - '/app/route-edge', - '/api/hello-edge', + function runTests() { + it('should render routes marked with restriction marks without errors', async () => { + const routes = [ + // app client components pages + '/app/client', '/app/client-edge', + // app sever components pages + '/app/server', '/app/server-edge', - ]) - const pagesManifest = JSON.parse( - await next.readFile('.next/server/pages-manifest.json') - ) - const middlewareManifest = JSON.parse( - await next.readFile('.next/server/middleware-manifest.json') - ) - expect(middlewareManifest.middleware).toBeTruthy() - expect(pagesManifest).toContainKeys([ - '/api/hello-edge', - '/pages-ssr', + // app routes + '/app/route', + '/app/route-edge', + // pages/api '/api/hello', - ]) + '/api/hello-edge', + '/api/mixed', + ] + + for (const route of routes) { + const { status } = await next.fetch(route) + expect([route, status]).toEqual([route, 200]) + } }) + + if (isNextStart) { + it('should log the build info properly', async () => { + const cliOutput = next.cliOutput + expect(cliOutput).toContain('Middleware') + + const functionsManifest = JSON.parse( + await next.readFile('.next/server/functions-config-manifest.json') + ) + expect(functionsManifest.functions).toContainKeys([ + '/app/route-edge', + '/api/hello-edge', + '/app/client-edge', + '/app/server-edge', + ]) + const pagesManifest = JSON.parse( + await next.readFile('.next/server/pages-manifest.json') + ) + const middlewareManifest = JSON.parse( + await next.readFile('.next/server/middleware-manifest.json') + ) + expect(middlewareManifest.middleware).toBeTruthy() + expect(pagesManifest).toContainKeys([ + '/api/hello-edge', + '/pages-ssr', + '/api/hello', + ]) + }) + } } + + describe('no server-only in server targets', () => { + const middlewareFile = 'middleware.js' + // const pagesApiFile = 'pages/api/hello.js' + let middlewareContent = '' + // let pagesApiContent = '' + + beforeAll(async () => { + await next.stop() + + middlewareContent = await next.readFile(middlewareFile) + // pagesApiContent = await next.readFile(pagesApiFile) + + 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'" + // ) + // ) + + await next.start() + }) + afterAll(async () => { + await next.patchFile(middlewareFile, middlewareContent) + // await next.patchFile(pagesApiFile, pagesApiContent) + }) + runTests() + }) + describe('with server-only in server targets', () => { + runTests() + }) } ) diff --git a/test/e2e/module-layer/lib/mixed-lib/client.js b/test/e2e/module-layer/lib/mixed-lib/client.js new file mode 100644 index 0000000000000..6839eda3e75b4 --- /dev/null +++ b/test/e2e/module-layer/lib/mixed-lib/client.js @@ -0,0 +1,3 @@ +import 'client-only' + +export const client = 'client:module' diff --git a/test/e2e/module-layer/lib/mixed-lib/index.js b/test/e2e/module-layer/lib/mixed-lib/index.js new file mode 100644 index 0000000000000..9dfb4806d1ba9 --- /dev/null +++ b/test/e2e/module-layer/lib/mixed-lib/index.js @@ -0,0 +1,3 @@ +export { shared as sharedComponentValue } from './shared' +// export but won't be consumed in the test +export { client as clientComponentValue } from './client' diff --git a/test/e2e/module-layer/lib/mixed-lib/shared.js b/test/e2e/module-layer/lib/mixed-lib/shared.js new file mode 100644 index 0000000000000..bbc74c049d7c5 --- /dev/null +++ b/test/e2e/module-layer/lib/mixed-lib/shared.js @@ -0,0 +1 @@ +export const shared = 'shared:module' diff --git a/test/e2e/module-layer/middleware.js b/test/e2e/module-layer/middleware.js index 4780d4b6425b8..5a1050e4c6651 100644 --- a/test/e2e/module-layer/middleware.js +++ b/test/e2e/module-layer/middleware.js @@ -1,6 +1,7 @@ import 'server-only' import { NextResponse } from 'next/server' +// import './lib/mixed-lib' -export function middleware() { +export function middleware(request) { return NextResponse.next() } diff --git a/test/e2e/module-layer/pages/api/hello-edge.js b/test/e2e/module-layer/pages/api/hello-edge.js index adcc549260798..dce0100296d19 100644 --- a/test/e2e/module-layer/pages/api/hello-edge.js +++ b/test/e2e/module-layer/pages/api/hello-edge.js @@ -1,7 +1,7 @@ import 'server-only' export default function handler() { - return new Response('api/hello-edge.js') + return new Response('pages/api/hello-edge.js:') } export const runtime = 'edge' diff --git a/test/e2e/module-layer/pages/api/hello.js b/test/e2e/module-layer/pages/api/hello.js index 3779e0be22f39..b806023b05a2c 100644 --- a/test/e2e/module-layer/pages/api/hello.js +++ b/test/e2e/module-layer/pages/api/hello.js @@ -1,5 +1,5 @@ import 'server-only' export default function handler(req, res) { - return res.send('api/hello.js') + return res.send('pages/api/hello.js:') } diff --git a/test/e2e/module-layer/pages/api/mixed.js b/test/e2e/module-layer/pages/api/mixed.js new file mode 100644 index 0000000000000..d49398953e56a --- /dev/null +++ b/test/e2e/module-layer/pages/api/mixed.js @@ -0,0 +1,5 @@ +import '../../lib/mixed-lib' + +export default function handler(req, res) { + return res.send('pages/api/mixed.js:') +}