Skip to content

Commit

Permalink
Fix CSS duplication related problems (#50406)
Browse files Browse the repository at this point in the history
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
vanilla-extract-css/vanilla-extract#1088 (comment).
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.

<img width="1141" alt="CleanShot-2023-05-26-GoB9Rhcs@2x"
src="https://github.com/vercel/next.js/assets/3676859/8e0f5346-ee64-4553-950a-7fb44f769efc">

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 #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.

<img width="1090" alt="CleanShot-2023-05-26-Ar4MN5rP@2x"
src="https://github.com/vercel/next.js/assets/3676859/4328d5d7-23af-4c42-bedf-30f8f062d96a">
  • Loading branch information
shuding committed May 30, 2023
1 parent e8bf785 commit 470e48c
Show file tree
Hide file tree
Showing 9 changed files with 165 additions and 15 deletions.
114 changes: 99 additions & 15 deletions packages/next/src/build/webpack/plugins/flight-client-entry-plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,66 @@ const pluginState = getProxiedPluginState({
injectedClientEntries: {} as Record<string, string>,
})

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<string>()
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
Expand Down Expand Up @@ -196,6 +256,8 @@ export class ClientReferenceEntryPlugin {
ClientComponentImports[0]
>()
const actionEntryImports = new Map<string, string[]>()
const clientEntriesToInject = []
const mergedCSSimports: CssImports = {}

for (const connection of compilation.moduleGraph.getOutgoingConnections(
entryModule
Expand All @@ -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,
Expand All @@ -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
Expand All @@ -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] ||
[]),
],
})
)
}
Expand Down Expand Up @@ -453,6 +531,8 @@ export class ClientReferenceEntryPlugin {
forEachEntryModule(compilation, ({ name, entryModule }) => {
const clientEntryDependencyMap = collectClientEntryDependencyMap(name)
const tracked = new Set<string>()
const mergedCSSimports: CssImports = {}

for (const connection of compilation.moduleGraph.getOutgoingConnections(
entryModule
)) {
Expand All @@ -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)
)
})
})

Expand Down Expand Up @@ -540,8 +625,8 @@ export class ClientReferenceEntryPlugin {
dependency: any /* Dependency */
clientEntryDependencyMap?: Record<string, any>
}): {
clientImports: ClientComponentImports
cssImports: CssImports
clientComponentImports: ClientComponentImports
actionImports: [string, string[]][]
clientActionImports: [string, string[]][]
} {
Expand Down Expand Up @@ -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)
}
Expand All @@ -643,7 +727,7 @@ export class ClientReferenceEntryPlugin {
}

return {
clientImports: clientComponentImports,
clientComponentImports,
cssImports: CSSImports.size
? {
[entryRequest]: Array.from(CSSImports),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import { Button } from './button'
import styles from './blue-button.module.css'

export function BlueButton() {
return <Button className={'btn-blue ' + styles['blue-button']}>Button</Button>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
.blue-button {
color: white;
background: blue;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import styles from './button.module.css'

export function Button({ className = '' }) {
return <div className={'btn ' + styles.button + ' ' + className}>Button</div>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
.button {
display: inline-block;
border: 1px solid black;
background: white;
}
12 changes: 12 additions & 0 deletions test/e2e/app-dir/app-css/app/css/css-conflict-layers/layout.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import { BlueButton } from './blue-button'

export default function ServerLayout({ children }) {
return (
<>
<div>
Blue Button: <BlueButton />
</div>
{children}
</>
)
}
3 changes: 3 additions & 0 deletions test/e2e/app-dir/app-css/app/css/css-conflict-layers/page.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
body {
font-size: large;
}
13 changes: 13 additions & 0 deletions test/e2e/app-dir/app-css/app/css/css-conflict-layers/page.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import './page.css'

import { Button } from './button'

export default function Page() {
return (
<>
<div>
Button: <Button />
</div>
</>
)
}
18 changes: 18 additions & 0 deletions test/e2e/app-dir/app-css/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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')

Expand Down

0 comments on commit 470e48c

Please sign in to comment.