From 470e48c00197e382ed4d76591e8f3d17baee71de Mon Sep 17 00:00:00 2001 From: Shu Ding Date: Tue, 30 May 2023 13:55:10 +0200 Subject: [PATCH] Fix CSS duplication related problems (#50406) This PR fixes a couple of categories of CSS issues in App Router, that come from the same root cause. ### 1. Duplicated styles being loaded in different layers This issue has been described in https://github.com/vanilla-extract-css/vanilla-extract/issues/1088#issuecomment-1563931144. If a CSS module (or a global CSS) is referenced in multiple layers (e.g. a layout and a page), it will be bundled into multiple CSS assets because each layer is considered as a separate entry. CleanShot-2023-05-26-GoB9Rhcs@2x As explained in that issue, we have to bundle all CSS modules into one chunk to avoid a big number of requests. ### 2. CSS ordering issues (conflicts) This is likely causing https://github.com/vercel/next.js/issues/48120. When the layer-based bundling and ordering logic applies to CSS, it can potentially cause non-deterministic order. In this example, button A in the layout should be in blue. However when button B is imported by the page, button A becomes red. This is an inconsistent experience and can be hard to debug and fix. CleanShot-2023-05-26-Ar4MN5rP@2x --- .../plugins/flight-client-entry-plugin.ts | 114 +++++++++++++++--- .../css/css-conflict-layers/blue-button.js | 6 + .../blue-button.module.css | 4 + .../app/css/css-conflict-layers/button.js | 5 + .../css/css-conflict-layers/button.module.css | 5 + .../app/css/css-conflict-layers/layout.js | 12 ++ .../app/css/css-conflict-layers/page.css | 3 + .../app/css/css-conflict-layers/page.js | 13 ++ test/e2e/app-dir/app-css/index.test.ts | 18 +++ 9 files changed, 165 insertions(+), 15 deletions(-) create mode 100644 test/e2e/app-dir/app-css/app/css/css-conflict-layers/blue-button.js create mode 100644 test/e2e/app-dir/app-css/app/css/css-conflict-layers/blue-button.module.css create mode 100644 test/e2e/app-dir/app-css/app/css/css-conflict-layers/button.js create mode 100644 test/e2e/app-dir/app-css/app/css/css-conflict-layers/button.module.css create mode 100644 test/e2e/app-dir/app-css/app/css/css-conflict-layers/layout.js create mode 100644 test/e2e/app-dir/app-css/app/css/css-conflict-layers/page.css create mode 100644 test/e2e/app-dir/app-css/app/css/css-conflict-layers/page.js 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 d89fc71c9e9c0..ee89870af8c06 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 @@ -92,6 +92,66 @@ const pluginState = getProxiedPluginState({ injectedClientEntries: {} as Record, }) +function deduplicateCSSImportsForEntry(mergedCSSimports: CssImports) { + // If multiple entry module connections are having the same CSS import, + // we only need to have one module to keep track of that CSS import. + // It is based on the fact that if a page or a layout is rendered in the + // given entry, all its parent layouts are always rendered too. + // This can avoid duplicate CSS imports in the generated CSS manifest, + // for example, if a page and its parent layout are both using the same + // CSS import, we only need to have the layout to keep track of that CSS + // import. + // To achieve this, we need to first collect all the CSS imports from + // every connection, and deduplicate them in the order of layers from + // top to bottom. The implementation can be generally described as: + // - Sort by number of `/` in the request path (the more `/`, the deeper) + // - When in the same depth, sort by the filename (template < layout < page and others) + + // Sort the connections as described above. + const sortedCSSImports = Object.entries(mergedCSSimports).sort((a, b) => { + const [aPath] = a + const [bPath] = b + + const aDepth = aPath.split('/').length + const bDepth = bPath.split('/').length + + if (aDepth !== bDepth) { + return aDepth - bDepth + } + + const aName = path.parse(aPath).name + const bName = path.parse(bPath).name + + const indexA = ['template', 'layout'].indexOf(aName) + const indexB = ['template', 'layout'].indexOf(bName) + + if (indexA === -1) return 1 + if (indexB === -1) return -1 + return indexA - indexB + }) + + const dedupedCSSImports: CssImports = {} + const trackedCSSImports = new Set() + for (const [entryName, cssImports] of sortedCSSImports) { + for (const cssImport of cssImports) { + if (trackedCSSImports.has(cssImport)) continue + + // Only track CSS imports that are in files that can inherit CSS. + const filename = path.parse(entryName).name + if (['template', 'layout'].includes(filename)) { + trackedCSSImports.add(cssImport) + } + + if (!dedupedCSSImports[entryName]) { + dedupedCSSImports[entryName] = [] + } + dedupedCSSImports[entryName].push(cssImport) + } + } + + return dedupedCSSImports +} + export class ClientReferenceEntryPlugin { dev: boolean appDir: string @@ -196,6 +256,8 @@ export class ClientReferenceEntryPlugin { ClientComponentImports[0] >() const actionEntryImports = new Map() + const clientEntriesToInject = [] + const mergedCSSimports: CssImports = {} for (const connection of compilation.moduleGraph.getOutgoingConnections( entryModule @@ -204,7 +266,7 @@ export class ClientReferenceEntryPlugin { const entryDependency = connection.dependency const entryRequest = connection.dependency.request - const { clientImports, actionImports } = + const { clientComponentImports, actionImports, cssImports } = this.collectComponentInfoFromDependencies({ entryRequest, compilation, @@ -219,7 +281,7 @@ export class ClientReferenceEntryPlugin { // Next.js internals are put into a separate entry. if (!isAbsoluteRequest) { - clientImports.forEach((value) => + clientComponentImports.forEach((value) => internalClientComponentEntryImports.add(value) ) continue @@ -240,14 +302,30 @@ export class ClientReferenceEntryPlugin { relativeRequest.replace(/\.[^.\\/]+$/, '').replace(/^src[\\/]/, '') ) + Object.assign(mergedCSSimports, cssImports) + clientEntriesToInject.push({ + compiler, + compilation, + entryName: name, + clientComponentImports, + cssImports, + bundlePath, + absolutePagePath: entryRequest, + }) + } + + // Make sure CSS imports are deduplicated before injecting the client entry + // and SSR modules. + const dedupedCSSImports = deduplicateCSSImportsForEntry(mergedCSSimports) + for (const clientEntryToInject of clientEntriesToInject) { addClientEntryAndSSRModulesList.push( this.injectClientEntryAndSSRModules({ - compiler, - compilation, - entryName: name, - clientImports, - bundlePath, - absolutePagePath: entryRequest, + ...clientEntryToInject, + clientImports: [ + ...clientEntryToInject.clientComponentImports, + ...(dedupedCSSImports[clientEntryToInject.absolutePagePath] || + []), + ], }) ) } @@ -453,6 +531,8 @@ export class ClientReferenceEntryPlugin { forEachEntryModule(compilation, ({ name, entryModule }) => { const clientEntryDependencyMap = collectClientEntryDependencyMap(name) const tracked = new Set() + const mergedCSSimports: CssImports = {} + for (const connection of compilation.moduleGraph.getOutgoingConnections( entryModule )) { @@ -471,9 +551,14 @@ export class ClientReferenceEntryPlugin { clientEntryDependencyMap, }) - if (!cssManifest.cssImports) cssManifest.cssImports = {} - Object.assign(cssManifest.cssImports, cssImports) + Object.assign(mergedCSSimports, cssImports) } + + if (!cssManifest.cssImports) cssManifest.cssImports = {} + Object.assign( + cssManifest.cssImports, + deduplicateCSSImportsForEntry(mergedCSSimports) + ) }) }) @@ -540,8 +625,8 @@ export class ClientReferenceEntryPlugin { dependency: any /* Dependency */ clientEntryDependencyMap?: Record }): { - clientImports: ClientComponentImports cssImports: CssImports + clientComponentImports: ClientComponentImports actionImports: [string, string[]][] clientActionImports: [string, string[]][] } { @@ -611,13 +696,12 @@ export class ClientReferenceEntryPlugin { CSSImports.add(modRequest) } - // Check if request is for css file. - if ((!inClientComponentBoundary && isClientComponent) || isCSS) { + if (!inClientComponentBoundary && isClientComponent) { clientComponentImports.push(modRequest) // Here we are entering a client boundary, and we need to collect dependencies // in the client graph too. - if (isClientComponent && clientEntryDependencyMap) { + if (clientEntryDependencyMap) { if (clientEntryDependencyMap[modRequest]) { filterClientComponents(clientEntryDependencyMap[modRequest], true) } @@ -643,7 +727,7 @@ export class ClientReferenceEntryPlugin { } return { - clientImports: clientComponentImports, + clientComponentImports, cssImports: CSSImports.size ? { [entryRequest]: Array.from(CSSImports), diff --git a/test/e2e/app-dir/app-css/app/css/css-conflict-layers/blue-button.js b/test/e2e/app-dir/app-css/app/css/css-conflict-layers/blue-button.js new file mode 100644 index 0000000000000..e4db41d232873 --- /dev/null +++ b/test/e2e/app-dir/app-css/app/css/css-conflict-layers/blue-button.js @@ -0,0 +1,6 @@ +import { Button } from './button' +import styles from './blue-button.module.css' + +export function BlueButton() { + return +} diff --git a/test/e2e/app-dir/app-css/app/css/css-conflict-layers/blue-button.module.css b/test/e2e/app-dir/app-css/app/css/css-conflict-layers/blue-button.module.css new file mode 100644 index 0000000000000..0cbe4551ec7d6 --- /dev/null +++ b/test/e2e/app-dir/app-css/app/css/css-conflict-layers/blue-button.module.css @@ -0,0 +1,4 @@ +.blue-button { + color: white; + background: blue; +} diff --git a/test/e2e/app-dir/app-css/app/css/css-conflict-layers/button.js b/test/e2e/app-dir/app-css/app/css/css-conflict-layers/button.js new file mode 100644 index 0000000000000..04c5980ca6487 --- /dev/null +++ b/test/e2e/app-dir/app-css/app/css/css-conflict-layers/button.js @@ -0,0 +1,5 @@ +import styles from './button.module.css' + +export function Button({ className = '' }) { + return
Button
+} diff --git a/test/e2e/app-dir/app-css/app/css/css-conflict-layers/button.module.css b/test/e2e/app-dir/app-css/app/css/css-conflict-layers/button.module.css new file mode 100644 index 0000000000000..9afc6b501074f --- /dev/null +++ b/test/e2e/app-dir/app-css/app/css/css-conflict-layers/button.module.css @@ -0,0 +1,5 @@ +.button { + display: inline-block; + border: 1px solid black; + background: white; +} diff --git a/test/e2e/app-dir/app-css/app/css/css-conflict-layers/layout.js b/test/e2e/app-dir/app-css/app/css/css-conflict-layers/layout.js new file mode 100644 index 0000000000000..ee4defadf9a15 --- /dev/null +++ b/test/e2e/app-dir/app-css/app/css/css-conflict-layers/layout.js @@ -0,0 +1,12 @@ +import { BlueButton } from './blue-button' + +export default function ServerLayout({ children }) { + return ( + <> +
+ Blue Button: +
+ {children} + + ) +} diff --git a/test/e2e/app-dir/app-css/app/css/css-conflict-layers/page.css b/test/e2e/app-dir/app-css/app/css/css-conflict-layers/page.css new file mode 100644 index 0000000000000..3a94c07339f95 --- /dev/null +++ b/test/e2e/app-dir/app-css/app/css/css-conflict-layers/page.css @@ -0,0 +1,3 @@ +body { + font-size: large; +} diff --git a/test/e2e/app-dir/app-css/app/css/css-conflict-layers/page.js b/test/e2e/app-dir/app-css/app/css/css-conflict-layers/page.js new file mode 100644 index 0000000000000..287988b3eef91 --- /dev/null +++ b/test/e2e/app-dir/app-css/app/css/css-conflict-layers/page.js @@ -0,0 +1,13 @@ +import './page.css' + +import { Button } from './button' + +export default function Page() { + return ( + <> +
+ Button:
+ + ) +} diff --git a/test/e2e/app-dir/app-css/index.test.ts b/test/e2e/app-dir/app-css/index.test.ts index 1b92b2aafbeb4..74ea957e387a5 100644 --- a/test/e2e/app-dir/app-css/index.test.ts +++ b/test/e2e/app-dir/app-css/index.test.ts @@ -347,6 +347,24 @@ createNextDescribe( ).toBe(1) }) + it('should deduplicate styles on the module level', async () => { + const browser = await next.browser('/css/css-conflict-layers') + await check( + () => + browser.eval( + `window.getComputedStyle(document.querySelector('.btn:not(.btn-blue)')).backgroundColor` + ), + 'rgb(255, 255, 255)' + ) + await check( + () => + browser.eval( + `window.getComputedStyle(document.querySelector('.btn.btn-blue')).backgroundColor` + ), + 'rgb(0, 0, 255)' + ) + }) + it('should only include the same style once in the flight data', async () => { const initialHtml = await next.render('/css/css-duplicate-2/server')