Skip to content

Commit

Permalink
Fix CSS imports tree-shaking (vercel#41357)
Browse files Browse the repository at this point in the history
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)
  • Loading branch information
shuding authored and Kikobeats committed Oct 24, 2022
1 parent 8c3cd09 commit caed5b5
Show file tree
Hide file tree
Showing 5 changed files with 110 additions and 17 deletions.
8 changes: 6 additions & 2 deletions packages/next/build/webpack/loaders/next-app-loader.ts
Expand Up @@ -33,6 +33,7 @@ async function createTreeCodeFromPath({
}) {
const splittedPath = pagePath.split(/[\\/]/)
const appDirPrefix = splittedPath[0]
const pages: string[] = []

async function createSubtreePropsFromSegmentPath(
segments: string[]
Expand All @@ -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
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -173,14 +176,15 @@ const nextAppLoader: webpack.LoaderDefinitionFunction<{
}
}

const treeCode = await createTreeCodeFromPath({
const [treeCode, pages] = await createTreeCodeFromPath({
pagePath,
resolve: resolver,
resolveParallelSegments,
})

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
Expand Down
53 changes: 50 additions & 3 deletions 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,
Expand Down Expand Up @@ -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, '/')}`
Expand Down Expand Up @@ -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<string, string[]> = {}

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<webpack.NormalModule>
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<string, string[]> =
flightCSSManifest.__entry_css__ || {}
entryCSSInfo[entryName] = cssImportsForChunk[entryName]

Object.assign(flightCSSManifest, {
__entry_css__: entryCSSInfo,
})
})
})

forEachEntryModule(({ entryModule }) => {
for (const connection of compilation.moduleGraph.getOutgoingConnections(
entryModule
Expand Down
4 changes: 4 additions & 0 deletions packages/next/build/webpack/plugins/flight-manifest-plugin.ts
Expand Up @@ -70,6 +70,10 @@ export type FlightManifest = {

export type FlightCSSManifest = {
[modulePath: string]: string[]
} & {
__entry_css__?: {
[entry: string]: string[]
}
}

const PLUGIN_NAME = 'FlightManifestPlugin'
Expand Down
59 changes: 49 additions & 10 deletions packages/next/server/app-render.tsx
Expand Up @@ -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] ||
Expand All @@ -525,24 +526,46 @@ function getCssInlinedLinkTags(
const chunks = new Set<string>()

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)
}
}
}
}

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 <link rel="preload" as="font"> tags based on server CSS manifest and font loader manifest. Only used when rendering to HTML.
*/
function getPreloadedFontFilesInlineLinkTags(
serverComponentManifest: FlightManifest,
serverCSSManifest: FlightCSSManifest,
fontLoaderManifest: FontLoaderManifest | undefined,
serverCSSForEntries: string[],
filePath?: string
): string[] {
if (!fontLoaderManifest || !filePath) {
Expand All @@ -559,10 +582,14 @@ function getPreloadedFontFilesInlineLinkTags(
const fontFiles = new Set<string>()

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)
}
}
}
}
Expand Down Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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
Expand Down
3 changes: 1 addition & 2 deletions test/e2e/app-dir/index.test.ts
Expand Up @@ -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'
Expand Down

0 comments on commit caed5b5

Please sign in to comment.