From caed5b58088c5064409dab27ee279f0311c2d1bb Mon Sep 17 00:00:00 2001 From: Shu Ding Date: Wed, 12 Oct 2022 15:41:19 +0200 Subject: [PATCH] Fix CSS imports tree-shaking (#41357) The way we currently track server CSS imports is to collect CSS files that each **module** depends on. This happens on the module graph level which is a global thing and cannot be tree-shaken properly (check the enabled test for more details). In this PR we collect another information, of CSS files that each **entrypoint** depends on. This is the CSS list after tree-shaken on the entry level. By intersecting these CSS imports with the module-level CSS imports, we can get the final used CSS imports for each _layout_. cc @hanneslund ## Bug - [ ] Related issues linked using `fixes #number` - [x] Integration tests added - [ ] Errors have a helpful link attached, see `contributing.md` ## Feature - [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR. - [ ] Related issues linked using `fixes #number` - [ ] Integration tests added - [ ] Documentation added - [ ] Telemetry added. In case of a feature if it's used or not. - [ ] Errors have a helpful link attached, see `contributing.md` ## Documentation / Examples - [ ] Make sure the linting passes by running `pnpm lint` - [ ] The "examples guidelines" are followed from [our contributing doc](https://github.com/vercel/next.js/blob/canary/contributing/examples/adding-examples.md) --- .../build/webpack/loaders/next-app-loader.ts | 8 ++- .../plugins/flight-client-entry-plugin.ts | 53 ++++++++++++++++- .../webpack/plugins/flight-manifest-plugin.ts | 4 ++ packages/next/server/app-render.tsx | 59 +++++++++++++++---- test/e2e/app-dir/index.test.ts | 3 +- 5 files changed, 110 insertions(+), 17 deletions(-) diff --git a/packages/next/build/webpack/loaders/next-app-loader.ts b/packages/next/build/webpack/loaders/next-app-loader.ts index b749afbbb5fc..3e724e56dd1f 100644 --- a/packages/next/build/webpack/loaders/next-app-loader.ts +++ b/packages/next/build/webpack/loaders/next-app-loader.ts @@ -33,6 +33,7 @@ async function createTreeCodeFromPath({ }) { const splittedPath = pagePath.split(/[\\/]/) const appDirPrefix = splittedPath[0] + const pages: string[] = [] async function createSubtreePropsFromSegmentPath( segments: string[] @@ -56,6 +57,8 @@ async function createTreeCodeFromPath({ if (parallelSegment === 'page') { const matchedPagePath = `${appDirPrefix}${parallelSegmentPath}` const resolvedPagePath = await resolve(matchedPagePath) + if (resolvedPagePath) pages.push(resolvedPagePath) + // Use '' for segment as it's the page. There can't be a segment called '' so this is the safest way to add it. props[parallelKey] = `['', {}, {layoutOrPagePath: ${JSON.stringify( resolvedPagePath @@ -107,7 +110,7 @@ async function createTreeCodeFromPath({ } const tree = await createSubtreePropsFromSegmentPath([]) - return `const tree = ${tree}.children;` + return [`const tree = ${tree}.children;`, pages] } function createAbsolutePath(appDir: string, pathToTurnAbsolute: string) { @@ -173,7 +176,7 @@ const nextAppLoader: webpack.LoaderDefinitionFunction<{ } } - const treeCode = await createTreeCodeFromPath({ + const [treeCode, pages] = await createTreeCodeFromPath({ pagePath, resolve: resolver, resolveParallelSegments, @@ -181,6 +184,7 @@ const nextAppLoader: webpack.LoaderDefinitionFunction<{ const result = ` export ${treeCode} + export const pages = ${JSON.stringify(pages)} export const AppRouter = require('next/dist/client/components/app-router.js').default export const LayoutRouter = require('next/dist/client/components/layout-router.js').default diff --git a/packages/next/build/webpack/plugins/flight-client-entry-plugin.ts b/packages/next/build/webpack/plugins/flight-client-entry-plugin.ts index 538f92ff87de..88d7f385f4a4 100644 --- a/packages/next/build/webpack/plugins/flight-client-entry-plugin.ts +++ b/packages/next/build/webpack/plugins/flight-client-entry-plugin.ts @@ -1,6 +1,5 @@ import { stringify } from 'querystring' import path from 'path' -import { relative } from 'path' import { webpack, sources } from 'next/dist/compiled/webpack/webpack' import { getInvalidator, @@ -96,7 +95,7 @@ export class FlightClientEntryPlugin { // Note that this isn't that reliable as webpack is still possible to assign // additional queries to make sure there's no conflict even using the `named` // module ID strategy. - let ssrNamedModuleId = relative(compiler.context, modResource) + let ssrNamedModuleId = path.relative(compiler.context, modResource) if (!ssrNamedModuleId.startsWith('.')) { // TODO use getModuleId instead ssrNamedModuleId = `./${ssrNamedModuleId.replace(/\\/g, '/')}` @@ -248,8 +247,56 @@ export class FlightClientEntryPlugin { ) }) - // After optimizing all the modules, we collect the CSS that are still used. + // After optimizing all the modules, we collect the CSS that are still used + // by the certain chunk. compilation.hooks.afterOptimizeModules.tap(PLUGIN_NAME, () => { + const cssImportsForChunk: Record = {} + + function collectModule(entryName: string, mod: any) { + const resource = mod.resource + if (resource) { + if (regexCSS.test(resource)) { + cssImportsForChunk[entryName].push(resource) + } + } + } + + compilation.chunkGroups.forEach((chunkGroup: any) => { + chunkGroup.chunks.forEach((chunk: webpack.Chunk) => { + // Here we only track page chunks. + if (!chunk.name) return + if (!chunk.name.endsWith('/page')) return + + const entryName = path.join(compiler.context, chunk.name) + + if (!cssImportsForChunk[entryName]) { + cssImportsForChunk[entryName] = [] + } + + const chunkModules = compilation.chunkGraph.getChunkModulesIterable( + chunk + ) as Iterable + for (const mod of chunkModules) { + collectModule(entryName, mod) + + const anyModule = mod as any + if (anyModule.modules) { + anyModule.modules.forEach((concatenatedMod: any) => { + collectModule(entryName, concatenatedMod) + }) + } + } + + const entryCSSInfo: Record = + flightCSSManifest.__entry_css__ || {} + entryCSSInfo[entryName] = cssImportsForChunk[entryName] + + Object.assign(flightCSSManifest, { + __entry_css__: entryCSSInfo, + }) + }) + }) + forEachEntryModule(({ entryModule }) => { for (const connection of compilation.moduleGraph.getOutgoingConnections( entryModule diff --git a/packages/next/build/webpack/plugins/flight-manifest-plugin.ts b/packages/next/build/webpack/plugins/flight-manifest-plugin.ts index c3432035bca5..436a9121244f 100644 --- a/packages/next/build/webpack/plugins/flight-manifest-plugin.ts +++ b/packages/next/build/webpack/plugins/flight-manifest-plugin.ts @@ -70,6 +70,10 @@ export type FlightManifest = { export type FlightCSSManifest = { [modulePath: string]: string[] +} & { + __entry_css__?: { + [entry: string]: string[] + } } const PLUGIN_NAME = 'FlightManifestPlugin' diff --git a/packages/next/server/app-render.tsx b/packages/next/server/app-render.tsx index 45fbb227e6d4..95bda6a2e64f 100644 --- a/packages/next/server/app-render.tsx +++ b/packages/next/server/app-render.tsx @@ -512,7 +512,8 @@ function getSegmentParam(segment: string): { function getCssInlinedLinkTags( serverComponentManifest: FlightManifest, serverCSSManifest: FlightCSSManifest, - filePath: string + filePath: string, + serverCSSForEntries: string[] ): string[] { const layoutOrPageCss = serverCSSManifest[filePath] || @@ -525,10 +526,14 @@ function getCssInlinedLinkTags( const chunks = new Set() for (const css of layoutOrPageCss) { - const mod = serverComponentManifest[css] - if (mod) { - for (const chunk of mod.default.chunks) { - chunks.add(chunk) + // We only include the CSS if it's a global CSS, or it is used by this + // entrypoint. + if (serverCSSForEntries.includes(css) || !/\.module\.css/.test(css)) { + const mod = serverComponentManifest[css] + if (mod) { + for (const chunk of mod.default.chunks) { + chunks.add(chunk) + } } } } @@ -536,6 +541,23 @@ function getCssInlinedLinkTags( return [...chunks] } +function getServerCSSForEntries( + serverCSSManifest: FlightCSSManifest, + entries: string[] +) { + const css = [] + for (const entry of entries) { + const entryName = entry.replace(/\.[^.]+$/, '') + if ( + serverCSSManifest.__entry_css__ && + serverCSSManifest.__entry_css__[entryName] + ) { + css.push(...serverCSSManifest.__entry_css__[entryName]) + } + } + return css +} + /** * Get inline tags based on server CSS manifest and font loader manifest. Only used when rendering to HTML. */ @@ -543,6 +565,7 @@ function getPreloadedFontFilesInlineLinkTags( serverComponentManifest: FlightManifest, serverCSSManifest: FlightCSSManifest, fontLoaderManifest: FontLoaderManifest | undefined, + serverCSSForEntries: string[], filePath?: string ): string[] { if (!fontLoaderManifest || !filePath) { @@ -559,10 +582,14 @@ function getPreloadedFontFilesInlineLinkTags( const fontFiles = new Set() for (const css of layoutOrPageCss) { - const preloadedFontFiles = fontLoaderManifest.app[css] - if (preloadedFontFiles) { - for (const fontFile of preloadedFontFiles) { - fontFiles.add(fontFile) + // We only include the CSS if it's a global CSS, or it is used by this + // entrypoint. + if (serverCSSForEntries.includes(css) || !/\.module\.css/.test(css)) { + const preloadedFontFiles = fontLoaderManifest.app[css] + if (preloadedFontFiles) { + for (const fontFile of preloadedFontFiles) { + fontFiles.add(fontFile) + } } } } @@ -908,6 +935,15 @@ export async function renderToHTMLOrFlight( let defaultRevalidate: false | undefined | number = false + // Collect all server CSS imports used by this specific entry (or entries, for parallel routes). + // Not that we can't rely on the CSS manifest because it tracks CSS imports per module, + // which can be used by multiple entries and cannot be tree-shaked in the module graph. + // More info: https://github.com/vercel/next.js/issues/41018 + const serverCSSForEntries = getServerCSSForEntries( + serverCSSManifest!, + ComponentMod.pages + ) + /** * Use the provided loader tree to create the React Component tree. */ @@ -941,13 +977,16 @@ export async function renderToHTMLOrFlight( ? getCssInlinedLinkTags( serverComponentManifest, serverCSSManifest!, - layoutOrPagePath + layoutOrPagePath, + serverCSSForEntries ) : [] + const preloadedFontFiles = getPreloadedFontFilesInlineLinkTags( serverComponentManifest, serverCSSManifest!, fontLoaderManifest, + serverCSSForEntries, layoutOrPagePath ) const Template = template diff --git a/test/e2e/app-dir/index.test.ts b/test/e2e/app-dir/index.test.ts index 18c1c6b8282f..ee420387eed2 100644 --- a/test/e2e/app-dir/index.test.ts +++ b/test/e2e/app-dir/index.test.ts @@ -1269,8 +1269,7 @@ describe('app dir', () => { ).toBe(false) }) - // TODO-APP: Should not include unused css modules in pages - it.skip('should not include unused css modules in nested pages in prod', async () => { + it('should not include unused css modules in nested pages in prod', async () => { const browser = await webdriver( next.url, '/css/css-page/unused-nested/inner'