Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix CSS duplication related problems #50406

Merged
merged 3 commits into from
May 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@ -234,14 +296,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 @@ -447,6 +525,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 @@ -465,9 +545,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 @@ -534,8 +619,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 @@ -605,13 +690,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 @@ -637,7 +721,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