From 3f11815b0247db14c68f2b4d5a6a7edac76ea83a Mon Sep 17 00:00:00 2001 From: Tobias Koppers Date: Fri, 5 Jul 2024 12:33:02 +0200 Subject: [PATCH] avoid merging global css in a way that leaks into other chunk groups (#67373) ### What? This disallows merging of global css with styles that appear on other pages/chunk groups. ### Why? Before we made the assumption that all CSS is written in a way that it only affects the elements it should really affect. In general writing CSS in that way is recommended. In App Router styles are only added and never removed. This means when a user uses client-side navigations to navigate the application, styles from all previous pages are still active on the current page. To avoid visual artefacts one need to write CSS in a way that it only affects certain elements. Usually this can be archived by using class names. CSS Modules even enforce this recommendation. Assuming that all styles are written this way allows to optimize CSS loading as request count can be reduced when (small) styles are merged together. But turns out that some applications are written differently. They use global styles that are not scoped to a class name (e. g. to `body` directly instead) and use them in different sections of the application. They are structured in a way that doesn't allow client-side navigations between these sections. This should be valid too, which makes our assumption not always holding true. This PR changes the algorithm so we only make that assumption for CSS Modules, but not for global CSS. While this affects the ability to optimize, applications usually do not use too much global CSS files, so that can be accepted. fixes #64773 --- .../webpack/plugins/css-chunking-plugin.ts | 34 +++++++++++++++++++ test/e2e/app-dir/css-order/app/base.css | 4 +++ .../css-order/app/global-first/page.tsx | 12 +++++++ .../css-order/app/global-first/style.css | 4 +++ .../css-order/app/global-second/page.tsx | 12 +++++++ .../css-order/app/global-second/style.css | 4 +++ test/e2e/app-dir/css-order/app/nav.tsx | 10 ++++++ test/e2e/app-dir/css-order/css-order.test.ts | 14 ++++++++ 8 files changed, 94 insertions(+) create mode 100644 test/e2e/app-dir/css-order/app/base.css create mode 100644 test/e2e/app-dir/css-order/app/global-first/page.tsx create mode 100644 test/e2e/app-dir/css-order/app/global-first/style.css create mode 100644 test/e2e/app-dir/css-order/app/global-second/page.tsx create mode 100644 test/e2e/app-dir/css-order/app/global-second/style.css diff --git a/packages/next/src/build/webpack/plugins/css-chunking-plugin.ts b/packages/next/src/build/webpack/plugins/css-chunking-plugin.ts index 3a2c48e9cf..a4699d04b7 100644 --- a/packages/next/src/build/webpack/plugins/css-chunking-plugin.ts +++ b/packages/next/src/build/webpack/plugins/css-chunking-plugin.ts @@ -11,6 +11,10 @@ const MIN_CSS_CHUNK_SIZE = 30 * 1024 */ const MAX_CSS_CHUNK_SIZE = 100 * 1024 +function isGlobalCss(module: Module) { + return !/\.module\.(css|scss|sass)$/.test(module.nameForCondition() || '') +} + type ChunkState = { chunk: Chunk modules: Module[] @@ -125,6 +129,8 @@ export class CssChunkingPlugin { // Process through all modules for (const startModule of remainingModules) { + let globalCssMode = isGlobalCss(startModule) + // The current position of processing in all selected chunks let allChunkStates = new Map(chunkStatesByModule.get(startModule)!) @@ -225,8 +231,36 @@ export class CssChunkingPlugin { } } } + + // Global CSS must not leak into unrelated chunks + const nextIsGlobalCss = isGlobalCss(nextModule) + if (nextIsGlobalCss && globalCssMode) { + if (allChunkStates.size !== nextChunkStates.size) { + // Fast check + continue + } + } + if (globalCssMode) { + for (const chunkState of nextChunkStates.keys()) { + if (!allChunkStates.has(chunkState)) { + // Global CSS would leak into chunkState + continue loop + } + } + } + if (nextIsGlobalCss) { + for (const chunkState of allChunkStates.keys()) { + if (!nextChunkStates.has(chunkState)) { + // Global CSS would leak into chunkState + continue loop + } + } + } potentialNextModules.delete(nextModule) currentSize += size + if (nextIsGlobalCss) { + globalCssMode = true + } for (const [chunkState, i] of nextChunkStates) { if (allChunkStates.has(chunkState)) { // This reduces the request count of the chunk group diff --git a/test/e2e/app-dir/css-order/app/base.css b/test/e2e/app-dir/css-order/app/base.css new file mode 100644 index 0000000000..af0ff0ad53 --- /dev/null +++ b/test/e2e/app-dir/css-order/app/base.css @@ -0,0 +1,4 @@ +#hello1, +#hello2 { + color: rgb(255, 0, 0); +} diff --git a/test/e2e/app-dir/css-order/app/global-first/page.tsx b/test/e2e/app-dir/css-order/app/global-first/page.tsx new file mode 100644 index 0000000000..400b6d2143 --- /dev/null +++ b/test/e2e/app-dir/css-order/app/global-first/page.tsx @@ -0,0 +1,12 @@ +import '../base.css' +import './style.css' +import Nav from '../nav' + +export default function Page() { + return ( +
+

hello world

+
+ ) +} diff --git a/test/e2e/app-dir/css-order/app/global-first/style.css b/test/e2e/app-dir/css-order/app/global-first/style.css new file mode 100644 index 0000000000..fc1883550e --- /dev/null +++ b/test/e2e/app-dir/css-order/app/global-first/style.css @@ -0,0 +1,4 @@ +#hello1, +#hello2 { + color: rgb(0, 255, 0); +} diff --git a/test/e2e/app-dir/css-order/app/global-second/page.tsx b/test/e2e/app-dir/css-order/app/global-second/page.tsx new file mode 100644 index 0000000000..5c7acc28a9 --- /dev/null +++ b/test/e2e/app-dir/css-order/app/global-second/page.tsx @@ -0,0 +1,12 @@ +import '../base.css' +import './style.css' +import Nav from '../nav' + +export default function Page() { + return ( +
+

hello world

+
+ ) +} diff --git a/test/e2e/app-dir/css-order/app/global-second/style.css b/test/e2e/app-dir/css-order/app/global-second/style.css new file mode 100644 index 0000000000..82401e75bd --- /dev/null +++ b/test/e2e/app-dir/css-order/app/global-second/style.css @@ -0,0 +1,4 @@ +#hello1, +#hello2 { + color: rgb(0, 0, 255); +} diff --git a/test/e2e/app-dir/css-order/app/nav.tsx b/test/e2e/app-dir/css-order/app/nav.tsx index d47adc22cf..d10d8e3618 100644 --- a/test/e2e/app-dir/css-order/app/nav.tsx +++ b/test/e2e/app-dir/css-order/app/nav.tsx @@ -70,6 +70,16 @@ export default function Nav() { Partial Reversed B +
  • + + Global First + +
    • +
  • + + Global Second + +

    • Pages