fix(app-router): isolate global-not-found chunk to fix CSS cascade (#1549)#1654
Conversation
…rridden by layout CSS (#1549) When `app/global-not-found.tsx` is defined, route-miss 404s render that module standalone (it ships its own `<html>`/`<body>` and replaces the root layout). Previously the entry registered global-not-found.tsx as a static `import * as`, which co-located its CSS with the root layout's CSS in a single chunk. Lightningcss then dropped overlapping declarations between the two stylesheets as dead code, so any rule global-not-found defined that the layout also defined was silently removed — breaking the CSS cascade for 404s. Switching the entry to `() => import("…/global-not-found.tsx")` puts the module in its own JS chunk and lets the SSR pipeline (and `Resources` metadata) treat its CSS independently of the rest of the app. The fallback renderer now accepts a `loadGlobalNotFoundModule` loader (cached per worker) instead of an eagerly-imported module. Also adds a regression test asserting the SSR markup for `/does-not-exist` links only global-not-found's stylesheet — never the root layout's — so the cascade can never resolve to the wrong color. Ported from Next.js: test/e2e/app-dir/initial-css-order/initial-css-order.test.ts
|
/bigbonk review |
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean fix. The root cause analysis is correct — static import * as puts global-not-found in the same chunk as the root layout, and lightningcss deduplicates overlapping declarations across concatenated sheets. Switching to dynamic import() gives the module its own chunk and its own CSS asset.
A few observations, nothing blocking:
Cached rejection promise. resolveGlobalNotFoundModule() caches the promise unconditionally. If the dynamic import() rejects (e.g. syntax error during dev), the rejected promise is cached forever for the lifetime of the worker/dev-server instance. The comment says "failures are surfaced on every call so they don't get swallowed" — that's true (every caller will see the rejection), but the module can never recover without restarting. In dev this is fine since HMR restarts the module graph, but worth noting. Not a blocker since the previous eager-import behavior had the same failure mode (module-level import error = worker crash).
Promise.resolve().then(loader) vs just loader(). The extra microtask hop via Promise.resolve().then(loadGlobalNotFoundModule) is harmless but unnecessary — loadGlobalNotFoundModule already returns a Promise (it's a dynamic import()). loadGlobalNotFoundModule() alone would work identically and be marginally cleaner. Totally cosmetic.
The test coverage is solid — the CSS link extraction helper, the positive/negative assertions on which stylesheets appear for matched vs 404 routes, and the unit test updates for the manifest and entry generator all cover the change well. The Next.js test reference is properly cited.
LGTM.
|
Review posted and PR approved. Here's a summary: Verdict: Approved The fix correctly identifies that a static Two minor observations (non-blocking):
Test coverage is thorough: CSS link extraction, positive/negative stylesheet assertions for matched vs 404 routes, and updated unit tests for the manifest and entry generator. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const globalNotFoundModule = ${globalNotFoundVar ? globalNotFoundVar : "null"}; | ||
| // See Next.js test: test/e2e/app-dir/initial-css-order/initial-css-order.test.ts | ||
| const __loadGlobalNotFoundModule = ${ | ||
| globalNotFoundImportSpecifier ? `() => import(${globalNotFoundImportSpecifier})` : "null" |
2 participants
Summary
global-not-found.tsx#1549.app/global-not-found.tsxis defined, route-miss 404s render that module standalone (skipping the root layout). The entry was registering it as a staticimport * as, so the bundler co-located its CSS with the root layout's CSS in a single chunk. Lightningcss then dropped overlapping declarations between the two stylesheets as dead code, breaking the CSS cascade — the layout's rules silently overrode global-not-found's.() => import("…/global-not-found.tsx")so the module gets its own JS chunk, and have the fallback renderer accept a (cached) loader instead of the eagerly-imported module./does-not-existlinks only global-not-found's stylesheet — never the layout's — so the cascade can never resolve to the wrong color.Ported from Next.js:
test/e2e/app-dir/initial-css-order/initial-css-order.test.ts.Test plan
vp test run tests/app-fallback-renderer.test.ts tests/entry-templates.test.ts tests/nextjs-compat/global-not-found.test.ts— all 41 tests pass (including the new CSS-link-order test).vp test run tests/app-router.test.ts— full app-router suite (332 tests) still passes.vp check— types and lints clean.