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 imports tree-shaking #41357

Merged
merged 2 commits into from Oct 12, 2022
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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
@@ -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) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a helper traverseModules in flight manifest plugin, consider using it to replace the manual chunk/module travsersal?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're not identical though (this one has early exits), but happy to refactor those later!

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