Skip to content

Commit

Permalink
Fix CSS resources being duplicated in app dir (#44168)
Browse files Browse the repository at this point in the history
Currently, to get all the chunk files that contain a specific module in a build, we use `chunk.files`. However a module itself can be included by multiple chunks, or even chunks from different entries. Theoretically that's correct but in our architecture, we only need these chunks that are from the entry that is currently rendering.

One solution is to add a 2-level key (the entry name) to modules in flight manifest, but that introduces too much size overhead to the manifest. So instead we leverage the `__entry_css_files__` field to generate a list of all files for a specific entry, and then find the intersection set of `{CSSFilesForEntry, CSSFilesForModule}` to get the corresponding CSS files for a specific Next.js entry.

Also renamed `__entry_css__` to be more specific, and did some performance optimizations.

NEXT-297

## Bug

- [ ] Related issues linked using `fixes #number`
- [x] Integration tests added
- [ ] Errors have a helpful link attached, see [`contributing.md`](https://github.com/vercel/next.js/blob/canary/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`
- [ ] [e2e](https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs) 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`](https://github.com/vercel/next.js/blob/canary/contributing.md)

## Documentation / Examples

- [ ] Make sure the linting passes by running `pnpm build && pnpm lint`
- [ ] The "examples guidelines" are followed from [our contributing doc](https://github.com/vercel/next.js/blob/canary/contributing/examples/adding-examples.md)
  • Loading branch information
shuding committed Dec 20, 2022
1 parent 04daf7e commit 966d2b1
Show file tree
Hide file tree
Showing 11 changed files with 111 additions and 46 deletions.
1 change: 1 addition & 0 deletions packages/next/build/webpack-config.ts
Expand Up @@ -2105,6 +2105,7 @@ export default async function getBaseWebpackConfig(
(isClient
? new FlightManifestPlugin({
dev,
appDir,
})
: new FlightClientEntryPlugin({
appDir,
Expand Down
Expand Up @@ -284,11 +284,11 @@ export class FlightClientEntryPlugin {
}

const entryCSSInfo: Record<string, string[]> =
cssManifest.__entry_css__ || {}
cssManifest.__entry_css_mods__ || {}
entryCSSInfo[entryName] = cssImportsForChunk[entryName]

Object.assign(cssManifest, {
__entry_css__: entryCSSInfo,
__entry_css_mods__: entryCSSInfo,
})
})
})
Expand Down Expand Up @@ -349,9 +349,9 @@ export class FlightClientEntryPlugin {
{
...serverCSSManifest,
...edgeServerCSSManifest,
__entry_css__: {
...serverCSSManifest.__entry_css__,
...edgeServerCSSManifest.__entry_css__,
__entry_css_mods__: {
...serverCSSManifest.__entry_css_mods__,
...edgeServerCSSManifest.__entry_css_mods__,
},
},
null,
Expand Down
66 changes: 41 additions & 25 deletions packages/next/build/webpack/plugins/flight-manifest-plugin.ts
Expand Up @@ -26,6 +26,7 @@ import { traverseModules } from '../utils'

interface Options {
dev: boolean
appDir: string
}

/**
Expand Down Expand Up @@ -65,14 +66,17 @@ export type FlightManifest = {
__edge_ssr_module_mapping__: {
[moduleId: string]: ManifestNode
}
__entry_css_files__: {
[entryPathWithoutExtension: string]: string[]
}
} & {
[modulePath: string]: ManifestNode
}

export type FlightCSSManifest = {
[modulePath: string]: string[]
} & {
__entry_css__?: {
__entry_css_mods__?: {
[entry: string]: string[]
}
}
Expand All @@ -86,9 +90,11 @@ export const ASYNC_CLIENT_MODULES = new Set<string>()

export class FlightManifestPlugin {
dev: Options['dev'] = false
appDir: Options['appDir']

constructor(options: Options) {
this.dev = options.dev
this.appDir = options.appDir
}

apply(compiler: webpack.Compiler) {
Expand Down Expand Up @@ -127,6 +133,7 @@ export class FlightManifestPlugin {
const manifest: FlightManifest = {
__ssr_module_mapping__: {},
__edge_ssr_module_mapping__: {},
__entry_css_files__: {},
}
const dev = this.dev

Expand All @@ -145,13 +152,10 @@ export class FlightManifestPlugin {
traverseModules(compilation, (mod) => collectClientRequest(mod))

compilation.chunkGroups.forEach((chunkGroup) => {
const cssResourcesInChunkGroup = new Set<string>()
let entryFilepath: string = ''

function recordModule(
chunk: webpack.Chunk,
id: ModuleId,
mod: webpack.NormalModule
mod: webpack.NormalModule,
chunkCSS: string[]
) {
const isCSSModule =
regexCSS.test(mod.resource) ||
Expand Down Expand Up @@ -191,30 +195,23 @@ export class FlightManifestPlugin {
ssrNamedModuleId = `./${ssrNamedModuleId.replace(/\\/g, '/')}`

if (isCSSModule) {
const chunks = [...chunk.files].filter(
(f) => !f.startsWith('static/css/pages/') && f.endsWith('.css')
)
if (!manifest[resource]) {
manifest[resource] = {
default: {
id,
name: 'default',
chunks,
chunks: chunkCSS,
},
}
} else {
// It is possible that there are multiple modules with the same resource,
// e.g. extracted by mini-css-extract-plugin. In that case we need to
// merge the chunks.
manifest[resource].default.chunks = [
...new Set([...manifest[resource].default.chunks, ...chunks]),
...new Set([...manifest[resource].default.chunks, ...chunkCSS]),
]
}

if (chunkGroup.name) {
cssResourcesInChunkGroup.add(resource)
}

return
}

Expand All @@ -224,10 +221,6 @@ export class FlightManifestPlugin {
return
}

if (/[\\/](page|layout)\.(ts|js)x?$/.test(resource)) {
entryFilepath = resource
}

const exportsInfo = compilation.moduleGraph.getExportsInfo(mod)
const isAsyncModule = ASYNC_CLIENT_MODULES.has(mod.resource)

Expand Down Expand Up @@ -328,27 +321,50 @@ export class FlightManifestPlugin {
chunk
// TODO: Update type so that it doesn't have to be cast.
) as Iterable<webpack.NormalModule>

const chunkCSS = [...chunk.files].filter(
(f) => !f.startsWith('static/css/pages/') && f.endsWith('.css')
)

for (const mod of chunkModules) {
const modId: string = compilation.chunkGraph.getModuleId(mod) + ''

recordModule(chunk, modId, mod)
recordModule(modId, mod, chunkCSS)

// If this is a concatenation, register each child to the parent ID.
// TODO: remove any
const anyModule = mod as any
if (anyModule.modules) {
anyModule.modules.forEach((concatenatedMod: any) => {
recordModule(chunk, modId, concatenatedMod)
recordModule(modId, concatenatedMod, chunkCSS)
})
}
}
})

const clientCSSManifest: any = manifest.__client_css_manifest__ || {}
if (entryFilepath) {
clientCSSManifest[entryFilepath] = Array.from(cssResourcesInChunkGroup)
const entryCSSFiles: any = manifest.__entry_css_files__ || {}

const addCSSFilesToEntry = (
files: string[],
entryName: string | undefined | null
) => {
if (entryName?.startsWith('app/')) {
const key = this.appDir + entryName.slice(3)
entryCSSFiles[key] = files.concat(entryCSSFiles[key] || [])
}
}

const cssFiles = chunkGroup.getFiles().filter((f) => f.endsWith('.css'))

if (cssFiles.length) {
// Add to chunk entry and parent chunk groups too.
addCSSFilesToEntry(cssFiles, chunkGroup.name)
chunkGroup.getParents().forEach((parent) => {
addCSSFilesToEntry(cssFiles, parent.options.name)
})
}
manifest.__client_css_manifest__ = clientCSSManifest

manifest.__entry_css_files__ = entryCSSFiles
})

const file = 'server/' + FLIGHT_MANIFEST
Expand Down
2 changes: 1 addition & 1 deletion packages/next/client/components/layout-router.tsx
Expand Up @@ -6,10 +6,10 @@ import type {
import type {
FlightRouterState,
FlightSegmentPath,
ChildProp,
} from '../../server/app-render'
import type { ErrorComponent } from './error-boundary'
import type { FocusAndScrollRef } from './reducer'
import type { ChildProp } from '../../server/app-render'

import React, { useContext, useEffect, use } from 'react'
import ReactDOM from 'react-dom'
Expand Down
32 changes: 18 additions & 14 deletions packages/next/server/app-render.tsx
Expand Up @@ -684,24 +684,28 @@ function getCssInlinedLinkTags(
filePath: string,
serverCSSForEntries: string[]
): string[] {
const layoutOrPageCss =
serverCSSManifest[filePath] ||
serverComponentManifest.__client_css_manifest__?.[filePath]
const layoutOrPageCssModules = serverCSSManifest[filePath]

if (!layoutOrPageCss) {
const filePathWithoutExt = filePath.replace(/\.[^.]+$/, '')
const cssFilesForEntry = new Set(
serverComponentManifest.__entry_css_files__?.[filePathWithoutExt] || []
)

if (!layoutOrPageCssModules || !cssFilesForEntry.size) {
return []
}

const chunks = new Set<string>()

for (const css of layoutOrPageCss) {
for (const mod of layoutOrPageCssModules) {
// 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)
if (serverCSSForEntries.includes(mod) || !/\.module\.css/.test(mod)) {
const modData = serverComponentManifest[mod]
if (modData) {
for (const chunk of modData.default.chunks) {
if (cssFilesForEntry.has(chunk)) {
chunks.add(chunk)
}
}
}
}
Expand All @@ -718,10 +722,10 @@ function getServerCSSForEntries(
for (const entry of entries) {
const entryName = entry.replace(/\.[^.]+$/, '')
if (
serverCSSManifest.__entry_css__ &&
serverCSSManifest.__entry_css__[entryName]
serverCSSManifest.__entry_css_mods__ &&
serverCSSManifest.__entry_css_mods__[entryName]
) {
css.push(...serverCSSManifest.__entry_css__[entryName])
css.push(...serverCSSManifest.__entry_css_mods__[entryName])
}
}
return css
Expand Down
3 changes: 2 additions & 1 deletion packages/next/server/dev/on-demand-entry-handler.ts
Expand Up @@ -2,6 +2,8 @@ import type ws from 'ws'
import origDebug from 'next/dist/compiled/debug'
import type { webpack } from 'next/dist/compiled/webpack/webpack'
import type { NextConfigComplete } from '../config-shared'
import type { DynamicParamTypesShort, FlightRouterState } from '../app-render'

import { EventEmitter } from 'events'
import { findPageFile } from '../lib/find-page-file'
import { runDependingOnPageType } from '../../build/entries'
Expand All @@ -15,7 +17,6 @@ import getRouteFromEntrypoint from '../get-route-from-entrypoint'
import { getPageStaticInfo } from '../../build/analysis/get-page-static-info'
import { isMiddlewareFile, isMiddlewareFilename } from '../../build/utils'
import { PageNotFoundError } from '../../shared/lib/utils'
import { DynamicParamTypesShort, FlightRouterState } from '../app-render'
import {
CompilerNameValues,
COMPILER_INDEXES,
Expand Down
5 changes: 5 additions & 0 deletions test/e2e/app-dir/app/app/css/css-duplicate/a/page.js
@@ -0,0 +1,5 @@
import Comp from '../comp'

export default function Page() {
return <Comp />
}
5 changes: 5 additions & 0 deletions test/e2e/app-dir/app/app/css/css-duplicate/b/page.js
@@ -0,0 +1,5 @@
import Comp from '../comp'

export default function Page() {
return <Comp />
}
7 changes: 7 additions & 0 deletions test/e2e/app-dir/app/app/css/css-duplicate/comp.js
@@ -0,0 +1,7 @@
'use client'

import './style.css'

export default function Comp() {
return <h1 className="red">Hello</h1>
}
3 changes: 3 additions & 0 deletions test/e2e/app-dir/app/app/css/css-duplicate/style.css
@@ -0,0 +1,3 @@
.red {
color: red;
}
23 changes: 23 additions & 0 deletions test/e2e/app-dir/index.test.ts
Expand Up @@ -1472,6 +1472,29 @@ createNextDescribe(
).toBe('50px')
})
})

if (isDev) {
describe('multiple entries', () => {
it('should only load chunks for the css module that is used by the specific entrypoint', async () => {
// Visit /b first
await next.render('/css/css-duplicate/b')

const browser = await next.browser('/css/css-duplicate/a')
expect(
await browser.eval(
`[...document.styleSheets].some(({ href }) => href.endsWith('/a/page.css'))`
)
).toBe(true)

// Should not load the chunk from /b
expect(
await browser.eval(
`[...document.styleSheets].some(({ href }) => href.endsWith('/b/page.css'))`
)
).toBe(false)
})
})
}
})

if (isDev) {
Expand Down

0 comments on commit 966d2b1

Please sign in to comment.