From 6790004d79ad17d1d512ee24d8cb5abead591e6e Mon Sep 17 00:00:00 2001 From: Shu Ding Date: Wed, 6 Dec 2023 00:17:31 +0800 Subject: [PATCH] Fix barrel optimization to ignore layers (#59254) Fixes #57624. The recent issue was an unexpected side effect caused by https://github.com/vercel/next.js/commit/305bb015060e82be3e6ae59a3d063c11a3e207f9, which only affects specific packages like `@mui/material`. The problem was that the entry file of `@mui/material` has `"use client"` at top, which affects the compilation result to output reference info only (when on the RSC layer), instead of keeping the original export statements. And the fix here is to ignore all layer info and React specific transforms here, as barrel optimization isn't related to all these framework features at all. To keep all directives unchanged, the SWC transform needs to parse and pass that info to the Webpack loader. This PR adds a test to ensure that `@mui/material` is working as expected (less than 1500 modules compiled). Without this feature it'll be ~2400 modules. Closes NEXT-1793, closes NEXT-1762. --- .../crates/core/src/optimize_barrel.rs | 42 +++++++++++- .../fixture/optimize-barrel/normal/4/input.js | 10 +++ .../optimize-barrel/normal/4/output.js | 5 +- packages/next/src/build/webpack-config.ts | 67 ++++++++++--------- .../webpack/loaders/next-barrel-loader.ts | 45 ++++++++----- .../next-flight-client-entry-loader.ts | 11 ++- .../plugins/flight-client-entry-plugin.ts | 6 +- .../barrel-optimization.test.ts | 22 ++++++ .../fixture/app/mui/page.js | 5 ++ test/turbopack-tests-manifest.json | 3 +- 10 files changed, 162 insertions(+), 54 deletions(-) create mode 100644 test/development/basic/barrel-optimization/fixture/app/mui/page.js diff --git a/packages/next-swc/crates/core/src/optimize_barrel.rs b/packages/next-swc/crates/core/src/optimize_barrel.rs index 6f0b26a03c..7224c9d735 100644 --- a/packages/next-swc/crates/core/src/optimize_barrel.rs +++ b/packages/next-swc/crates/core/src/optimize_barrel.rs @@ -73,11 +73,16 @@ impl Fold for OptimizeBarrel { // We only apply this optimization to barrel files. Here we consider // a barrel file to be a file that only exports from other modules. + // Besides that, lit expressions are allowed as well ("use client", etc.). + let mut allowed_directives = true; + let mut directives = vec![]; + let mut is_barrel = true; for item in &items { match item { ModuleItem::ModuleDecl(decl) => { + allowed_directives = false; match decl { ModuleDecl::Import(_) => {} // export { foo } from './foo'; @@ -198,10 +203,17 @@ impl Fold for OptimizeBarrel { } ModuleItem::Stmt(stmt) => match stmt { Stmt::Expr(expr) => match &*expr.expr { - Expr::Lit(_) => { - new_items.push(item.clone()); + Expr::Lit(l) => { + if let Lit::Str(s) = l { + if allowed_directives && s.value.starts_with("use ") { + directives.push(s.value.to_string()); + } + } else { + allowed_directives = false; + } } _ => { + allowed_directives = false; if !self.wildcard { is_barrel = false; break; @@ -209,6 +221,7 @@ impl Fold for OptimizeBarrel { } }, _ => { + allowed_directives = false; if !self.wildcard { is_barrel = false; break; @@ -259,6 +272,31 @@ impl Fold for OptimizeBarrel { type_only: false, }))); } + + // Push directives. + if !directives.is_empty() { + new_items.push(ModuleItem::ModuleDecl(ModuleDecl::ExportDecl(ExportDecl { + span: DUMMY_SP, + decl: Decl::Var(Box::new(VarDecl { + span: DUMMY_SP, + kind: VarDeclKind::Const, + declare: false, + decls: vec![VarDeclarator { + span: DUMMY_SP, + name: Pat::Ident(BindingIdent { + id: private_ident!("__next_private_directive_list__"), + type_ann: None, + }), + init: Some(Box::new(Expr::Lit(Lit::Str(Str { + span: DUMMY_SP, + value: serde_json::to_string(&directives).unwrap().into(), + raw: None, + })))), + definite: false, + }], + })), + }))); + } } new_items diff --git a/packages/next-swc/crates/core/tests/fixture/optimize-barrel/normal/4/input.js b/packages/next-swc/crates/core/tests/fixture/optimize-barrel/normal/4/input.js index bd460bc9a2..dc6d0c70c8 100644 --- a/packages/next-swc/crates/core/tests/fixture/optimize-barrel/normal/4/input.js +++ b/packages/next-swc/crates/core/tests/fixture/optimize-barrel/normal/4/input.js @@ -1,3 +1,9 @@ +/* Test */ +/* Test directives inside comments +'use server' +*/ + +// This should be kept 'use client' import foo, { a, b } from 'foo' @@ -7,3 +13,7 @@ export { a as x } export { y } from '1' export { b } export { foo as default, z } + +// This should be removed as it's not on top +// prettier-ignore +'use strict' diff --git a/packages/next-swc/crates/core/tests/fixture/optimize-barrel/normal/4/output.js b/packages/next-swc/crates/core/tests/fixture/optimize-barrel/normal/4/output.js index abe88ed8a7..4f0a733801 100644 --- a/packages/next-swc/crates/core/tests/fixture/optimize-barrel/normal/4/output.js +++ b/packages/next-swc/crates/core/tests/fixture/optimize-barrel/normal/4/output.js @@ -1,2 +1,5 @@ -'use client'; +/* Test */ /* Test directives inside comments +'use server' +*/ // This should be kept export const __next_private_export_map__ = '[["x","foo","a"],["y","1","y"],["b","foo","b"],["default","foo","default"],["z","bar","default"]]'; +export const __next_private_directive_list__ = '["use client"]'; \ No newline at end of file diff --git a/packages/next/src/build/webpack-config.ts b/packages/next/src/build/webpack-config.ts index cec79d499f..60dde7bbc8 100644 --- a/packages/next/src/build/webpack-config.ts +++ b/packages/next/src/build/webpack-config.ts @@ -746,7 +746,7 @@ export default async function getBaseWebpackConfig( config.experimental.externalDir || !!config.transpilePackages const codeCondition = { - test: /\.(tsx|ts|js|cjs|mjs|jsx)$/, + test: { or: [/\.(tsx|ts|js|cjs|mjs|jsx)$/, /__barrel_optimize__/] }, ...(shouldIncludeExternalDirs ? // Allowing importing TS/TSX files from outside of the root dir. {} @@ -1124,37 +1124,6 @@ export default async function getBaseWebpackConfig( }, module: { rules: [ - { - // This loader rule works like a bridge between user's import and - // the target module behind a package's barrel file. It reads SWC's - // analysis result from the previous loader, and directly returns the - // code that only exports values that are asked by the user. - test: /__barrel_optimize__/, - use: ({ resourceQuery }: { resourceQuery: string }) => { - const names = ( - resourceQuery.match(/\?names=([^&]+)/)?.[1] || '' - ).split(',') - - return [ - { - loader: 'next-barrel-loader', - options: { - names, - swcCacheDir: path.join( - dir, - config?.distDir ?? '.next', - 'cache', - 'swc' - ), - }, - // This is part of the request value to serve as the module key. - // The barrel loader are no-op re-exported modules keyed by - // export names. - ident: 'next-barrel-loader:' + resourceQuery, - }, - ] - }, - }, // Alias server-only and client-only to proper exports based on bundling layers { issuerLayer: { @@ -1580,6 +1549,40 @@ export default async function getBaseWebpackConfig( test: /[\\/]next[\\/]dist[\\/](esm[\\/])?server[\\/]og[\\/]image-response\.js/, sideEffects: false, }, + { + // This loader rule should be before other rules, as it can output code + // that still contains `"use client"` or `"use server"` statements that + // needs to be re-transformed by the RSC compilers. + // This loader rule works like a bridge between user's import and + // the target module behind a package's barrel file. It reads SWC's + // analysis result from the previous loader, and directly returns the + // code that only exports values that are asked by the user. + test: /__barrel_optimize__/, + use: ({ resourceQuery }: { resourceQuery: string }) => { + const names = ( + resourceQuery.match(/\?names=([^&]+)/)?.[1] || '' + ).split(',') + + return [ + { + loader: 'next-barrel-loader', + options: { + names, + swcCacheDir: path.join( + dir, + config?.distDir ?? '.next', + 'cache', + 'swc' + ), + }, + // This is part of the request value to serve as the module key. + // The barrel loader are no-op re-exported modules keyed by + // export names. + ident: 'next-barrel-loader:' + resourceQuery, + }, + ] + }, + }, ], }, plugins: [ diff --git a/packages/next/src/build/webpack/loaders/next-barrel-loader.ts b/packages/next/src/build/webpack/loaders/next-barrel-loader.ts index 18bc3162ce..2a3e7408d9 100644 --- a/packages/next/src/build/webpack/loaders/next-barrel-loader.ts +++ b/packages/next/src/build/webpack/loaders/next-barrel-loader.ts @@ -88,7 +88,6 @@ import type webpack from 'webpack' import path from 'path' import { transform } from '../../swc' -import { WEBPACK_LAYERS } from '../../../lib/constants' // This is a in-memory cache for the mapping of barrel exports. This only applies // to the packages that we optimize. It will never change (e.g. upgrading packages) @@ -97,14 +96,13 @@ import { WEBPACK_LAYERS } from '../../../lib/constants' const barrelTransformMappingCache = new Map< string, { - prefix: string exportList: [string, string, string][] wildcardExports: string[] + isClientEntry: boolean } | null >() async function getBarrelMapping( - layer: string | null | undefined, resourcePath: string, swcCacheDir: string, resolve: (context: string, request: string) => Promise, @@ -135,9 +133,6 @@ async function getBarrelMapping( optimizeBarrelExports: { wildcard: isWildcard, }, - serverComponents: { - isReactServerLayer: layer === WEBPACK_LAYERS.reactServerComponents, - }, jsc: { parser: { syntax: isTypeScript ? 'typescript' : 'ecmascript', @@ -155,7 +150,11 @@ async function getBarrelMapping( // Avoid circular `export *` dependencies const visited = new Set() - async function getMatches(file: string, isWildcard: boolean) { + async function getMatches( + file: string, + isWildcard: boolean, + isClientEntry: boolean + ) { if (visited.has(file)) { return null } @@ -180,7 +179,15 @@ async function getBarrelMapping( return null } - const prefix = matches[1] + const matchedDirectives = output.match( + /^([^]*)export (const|var) __next_private_directive_list__ = '([^']+)'/ + ) + const directiveList = matchedDirectives + ? JSON.parse(matchedDirectives[3]) + : [] + // "use client" in barrel files has to be transferred to the target file. + isClientEntry = directiveList.includes('use client') + let exportList = JSON.parse(matches[3].slice(1, -1)) as [ string, string, @@ -209,7 +216,11 @@ async function getBarrelMapping( req.replace('__barrel_optimize__?names=__PLACEHOLDER__!=!', '') ) - const targetMatches = await getMatches(targetPath, true) + const targetMatches = await getMatches( + targetPath, + true, + isClientEntry + ) if (targetMatches) { // Merge the export list exportList = exportList.concat(targetMatches.exportList) @@ -219,13 +230,13 @@ async function getBarrelMapping( } return { - prefix, exportList, wildcardExports, + isClientEntry, } } - const res = await getMatches(resourcePath, false) + const res = await getMatches(resourcePath, false, false) barrelTransformMappingCache.set(resourcePath, res) return res @@ -249,7 +260,6 @@ const NextBarrelLoader = async function ( }) const mapping = await getBarrelMapping( - this._module?.layer, this.resourcePath, swcCacheDir, resolve, @@ -271,15 +281,14 @@ const NextBarrelLoader = async function ( return } - // It needs to keep the prefix for comments and directives like "use client". - const prefix = mapping.prefix const exportList = mapping.exportList + const isClientEntry = mapping.isClientEntry const exportMap = new Map() for (const [name, filePath, orig] of exportList) { exportMap.set(name, [filePath, orig]) } - let output = prefix + let output = '' let missedNames: string[] = [] for (const name of names) { // If the name matches @@ -313,6 +322,12 @@ const NextBarrelLoader = async function ( } } + // When it has `"use client"` inherited from its barrel files, we need to + // prefix it to this target file as well. + if (isClientEntry) { + output = `"use client";\n${output}` + } + this.callback(null, output) } diff --git a/packages/next/src/build/webpack/loaders/next-flight-client-entry-loader.ts b/packages/next/src/build/webpack/loaders/next-flight-client-entry-loader.ts index 0be3b6fa2b..13690a20a0 100644 --- a/packages/next/src/build/webpack/loaders/next-flight-client-entry-loader.ts +++ b/packages/next/src/build/webpack/loaders/next-flight-client-entry-loader.ts @@ -1,4 +1,7 @@ -import { RSC_MODULE_TYPES } from '../../../shared/lib/constants' +import { + BARREL_OPTIMIZATION_PREFIX, + RSC_MODULE_TYPES, +} from '../../../shared/lib/constants' import { getModuleBuildInfo } from './get-module-build-info' import { regexCSS } from './utils' @@ -26,7 +29,11 @@ export default function transformSource(this: any) { .filter((request) => (isServer ? !regexCSS.test(request) : true)) .map( (request) => - `import(/* webpackMode: "eager" */ ${JSON.stringify(request)})` + `import(/* webpackMode: "eager" */ ${JSON.stringify( + request.startsWith(BARREL_OPTIMIZATION_PREFIX) + ? request.replace(':', '!=!') + : request + )})` ) .join(';\n') diff --git a/packages/next/src/build/webpack/plugins/flight-client-entry-plugin.ts b/packages/next/src/build/webpack/plugins/flight-client-entry-plugin.ts index b9014ad967..7e486baf0d 100644 --- a/packages/next/src/build/webpack/plugins/flight-client-entry-plugin.ts +++ b/packages/next/src/build/webpack/plugins/flight-client-entry-plugin.ts @@ -193,7 +193,11 @@ export class FlightClientEntryPlugin { const modQuery = mod.resourceResolveData?.query || '' // query is already part of mod.resource // so it's only necessary to add it for matchResource or mod.resourceResolveData - const modResource = modPath ? modPath + modQuery : mod.resource + const modResource = modPath + ? modPath.startsWith(BARREL_OPTIMIZATION_PREFIX) + ? mod.resource + : modPath + modQuery + : mod.resource if (mod.layer !== WEBPACK_LAYERS.serverSideRendering) { return diff --git a/test/development/basic/barrel-optimization/barrel-optimization.test.ts b/test/development/basic/barrel-optimization/barrel-optimization.test.ts index 3613cded5e..6fb86c5861 100644 --- a/test/development/basic/barrel-optimization/barrel-optimization.test.ts +++ b/test/development/basic/barrel-optimization/barrel-optimization.test.ts @@ -28,6 +28,9 @@ createNextDescribe( '@heroicons/react': '2.0.18', '@visx/visx': '3.3.0', 'recursive-barrel': '1.0.0', + '@mui/material': '5.14.19', + '@emotion/styled': '11.11.0', + '@emotion/react': '11.11.1', }, }, ({ next }) => { @@ -127,6 +130,25 @@ createNextDescribe( expect(html).toContain(' { + let logs = '' + next.on('stdout', (log) => { + logs += log + }) + + // Ensure that MUI is working + const html = await next.render('/mui') + expect(html).toContain('test_mui') + + const modules = [...logs.matchAll(/\((\d+) modules\)/g)] + expect(modules.length).toBeGreaterThanOrEqual(1) + for (const [, moduleCount] of modules) { + // Ensure that the number of modules is less than 1500 - otherwise we're + // importing the entire library. + expect(parseInt(moduleCount)).toBeLessThan(1500) + } + }) + it('should not break "use client" directive in optimized packages', async () => { const html = await next.render('/client') expect(html).toContain('this is a client component') diff --git a/test/development/basic/barrel-optimization/fixture/app/mui/page.js b/test/development/basic/barrel-optimization/fixture/app/mui/page.js new file mode 100644 index 0000000000..0a1913f870 --- /dev/null +++ b/test/development/basic/barrel-optimization/fixture/app/mui/page.js @@ -0,0 +1,5 @@ +import { Button } from '@mui/material' + +export default function Page() { + return +} diff --git a/test/turbopack-tests-manifest.json b/test/turbopack-tests-manifest.json index 4638dba61e..5bc1830d28 100644 --- a/test/turbopack-tests-manifest.json +++ b/test/turbopack-tests-manifest.json @@ -1459,7 +1459,8 @@ "optimizePackageImports app - should render the icons correctly without creating all the modules", "optimizePackageImports pages - should optimize recursive wildcard export mapping", "optimizePackageImports pages - should render the icons correctly without creating all the modules", - "optimizePackageImports should support visx" + "optimizePackageImports should support visx", + "optimizePackageImports should support MUI" ], "pending": [], "flakey": [],