fix: CSS modules not working in _app/_layout/_error and across routes in non-island components by Hajime-san · Pull Request #3781 · freshframework/fresh

Hajime-san · 2026-04-25T08:05:28Z

Originally reported:

CSS Modules compilation issues #3633 (comment)

Repro:
https://github.com/honmanyau/fresh-2-layout-css-module-issue/blob/24bf9c597a057c60acdb2c2934a65f22b6276ecd/routes/page-b/_layout.tsx

The PR seems to resolve the island components problem, but not non-island.

fix: CSS modules not working in _app/_layout/_error and across routes in dev #3764

This PR introduces CSS Modules handling for non-island components rendered through utility files such as _app, _layout, _error, and _404. The core change is that utility-file mod.css is now preserved through Fresh’s command and render pipeline, so CSS referenced from app/layout/route/error rendering can be accumulated and emitted in the final response instead of being dropped.

On the Vite side, development now also collects CSS exposed through fresh-route-css::* virtual modules, which is the path used by server-rendered non-island components in utility files. For production builds, CSS placeholder replacement is no longer limited to route chunks, because this CSS can be hoisted into shared chunks such as server-entry.

I added new tests and confirmed to work fine about our private project via links in deno config locally.

bartlomieju

Nice fix — the approach of threading css through the existing command → segment → context pipeline is clean and consistent with the rest of the architecture. A few things caught my eye below (inline), but overall this looks solid.

The setAdditionalStyles refactor from a simple setter to a dedup-accumulator is exactly right, getSsrModule() properly handles both real files and virtual module IDs, and the BFS graph walk in collectRouteCss is thorough.

Let me know if you'd like a hand getting this over the finish line — happy to help with any of the items below or with testing.

bartlomieju · 2026-04-25T12:32:34Z

+              `["__FRESH_CSS_PLACEHOLDER__"]`,
+              info.css
+                ? JSON.stringify(info.css.map((css) => `/${css}`))
+                : "null",


This uses content.replace() which only replaces the first occurrence of the placeholder. If two utility-file CSS placeholders (e.g. from both _app and _layout) get hoisted into the same shared chunk like server-entry, only the first one would be replaced.

Should this be replaceAll (or a loop) to be safe?

Alternatively, if each route virtual module gets its own unique placeholder, this is fine — but it might be worth a comment explaining why a single replace is sufficient.

Nice catch. I notice that it causes requesting to http://localhost:8000/__FRESH_CSS_PLACEHOLDER__ in build mode. It should be replaceAll. Fixed and added a test.

bartlomieju · 2026-04-25T12:32:34Z


+          route.css = server === undefined
+            ? route.css
+            : await collectRouteCss(server, route.filePath);


This re-walks the full SSR import graph and overwrites route.css on every load call (including HMR invalidations). That seems intentional for picking up changes during dev, but for deep dependency trees the fetchModule calls inside collectRouteCss could add up.

Might be worth a short comment here noting this is intentional for HMR correctness, so a future reader doesn't try to "optimize" it with caching.

Sorry for confusing, it seems to be my AI slop code. This code is unnecessary for HMR.
I'm not sure if the HMR is working with the changes in this PR. Since it definitely works after a page reload, I'm thinking of putting it out of scope for now.

bartlomieju · 2026-04-25T12:32:34Z

@@ -298,16 +321,18 @@ export class Context<State> {



Nit: appDef?.css evaluates to undefined when appDef is null, which the new setAdditionalStyles handles via the css == null guard. Works correctly, but it's a subtle null-propagation chain. An explicit guard might be clearer:

if (appDef !== null) { setAdditionalStyles(this, appDef.css); }

Especially since the appDef !== null checks already exist a few lines below.

bartlomieju · 2026-04-25T12:32:35Z

+
+      if (ctx.#additionalStyles === null) {
+        ctx.#additionalStyles = css.slice();
+        return;


Minor: includes() is O(n) per insertion. For the small CSS lists in practice this is fine, but a Set would be more idiomatic if you ever want to tighten this up. Low priority.

Yep, I added a FIXME comment below. I think it could be use Set instead of css: Array<string>.It seems that at least all the tests pass in the following branch.

Hajime-san/fresh@fix-ssr-css-modules...Hajime-san:fresh:perf-css

I'm planning it as a follow up PR.

bartlomieju · 2026-04-25T12:32:35Z

+        });
+      },
+    );
+  },


The tests cover _layout.tsx which is great. The PR title also mentions _app, _error, and _404 — are those paths already exercised by existing tests, or would it be worth adding a case or two here to lock in coverage for those as well?

Hajime-san · 2026-04-26T01:20:30Z

@bartlomieju
Thanks!
Upon investigation, I found that our project was exhibiting behavior where the screen was not rendering in dev mode.
Also, it appears that CI is crashing only on Windows. I will re-request review once the items already reviewed and the above problems are resolved.

UPDATE:

the screen was not rendering in dev mode.

It seems an another problem of links. It works fine with vendor and copy-pasting locally.

This reverts commit f1934f0.

Hajime-san · 2026-04-26T02:28:10Z

+              `["__FRESH_CSS_PLACEHOLDER__"]`,
+              info.css
+                ? JSON.stringify(info.css.map((css) => `/${css}`))
+                : "null",


Nice catch. I notice that it causes requesting to http://localhost:8000/__FRESH_CSS_PLACEHOLDER__ in build mode. It should be replaceAll. Fixed and added a test.

Hajime-san · 2026-04-26T11:03:10Z

    files.push({
      id,
-      filePath: entry.path,
+      filePath: toPosix(entry.path),


Suddenly, several tests failed on Windows. It's likely that a potential bug was occured by this PR.

vite dev - css modules => ./packages/fresh/src/test_utils.ts:198:8 vite dev - css modules in _app/_layout/_error non-island component are injected => ./packages/fresh/src/test_utils.ts:198:8 vite dev - route css import => ./packages/fresh/src/test_utils.ts:198:8

debug console on Windows

------- post-test output ------- 👺 > fresh:route-css > name _index <!DOCTYPE html> <html lang="en"> <head> <meta charset="utf-8"> <meta name="viewport" content="width=device-width, initial-scale=1.0"> </head> <body> <div class="green"> <h1 class="_root_1mdiz_1"> green text </h1> </div> <div class="red"> <h1 class="_root_xlv1v_1"> red text </h1> </div> <h1> ok </h1> </body> </html> [ "Port 5173 is in use, trying another one...", "", " VITE v7.3.1 ready in 1276 ms", "", " ➜ Local: http://127.0.0.1:5174/" ] [ "Port 5173 is in use, trying another one...", "", " VITE v7.3.1 ready in 1276 ms", "", " ➜ Local: http://127.0.0.1:5174/", "\x1b[2m \x1b[32m➜\x1b[39m \x1b[1mNetwork\x1b[22m\x1b[2m: use \x1b[22m\x1b[1m--host\x1b[22m\x1b[2m to expose\x1b[22m", "👺 > fresh:route-css > route {", ' id: "/_app",', ' filePath: "D:\\\\a\\\\fresh\\\\fresh\\\\packages\\\\plugin-vite\\\\tests\\\\fixtures\\\\non_island_css_modules\\\\routes\\\\_app.tsx",', ' type: "app",', ' pattern: "*",', ' routePattern: "*",', " lazy: false,", " css: [", ' "/@fs/D:/a/fresh/fresh/packages/plugin-vite/demo/components/CssModulesNonIsland.module.css"', " ],", " overrideConfig: undefined", "}", "👺 > fresh:route-css > name __app", "👺 > fresh:route-css > route {", ' id: "/_layout",', ' filePath: "D:\\\\a\\\\fresh\\\\fresh\\\\packages\\\\plugin-vite\\\\tests\\\\fixtures\\\\non_island_css_modules\\\\routes\\\\_layout.tsx",', ' type: "layout",', ' pattern: "/",', ' routePattern: "/",', " lazy: false,", " css: [", ' "/@fs/D:/a/fresh/fresh/packages/plugin-vite/demo/components/CssModulesNonIsland3.module.css"', " ],", " overrideConfig: undefined", "}", "👺 > fresh:route-css > name __layout", "👺 > fresh:route-css > route {", ' id: "/_error",', ' filePath: "D:\\\\a\\\\fresh\\\\fresh\\\\packages\\\\plugin-vite\\\\tests\\\\fixtures\\\\non_island_css_modules\\\\routes\\\\_error.tsx",', ' type: "error",', ' pattern: "/",', ' routePattern: "/",', " lazy: false,", " css: [", ' "/@fs/D:/a/fresh/fresh/packages/plugin-vite/demo/components/CssModulesNonIsland2.module.css"', " ],", " overrideConfig: undefined", "}", "👺 > fresh:route-css > name __error", "👺 > fresh:route-css > route {", ' id: "/index",', ' filePath: "D:\\\\a\\\\fresh\\\\fresh\\\\packages\\\\plugin-vite\\\\tests\\\\fixtures\\\\non_island_css_modules\\\\routes\\\\index.tsx",', ' type: "route",', ' pattern: "/",', ' routePattern: "/",', " lazy: true,", " css: [],", ' overrideConfig: { methods: "ALL" }', "}", "👺 > fresh:route-css > name _index", "👺 > renderRoute > setAdditionalStyles []", "👺 > render Compose VNode > setAdditionalStyles []", "👺 > render compose App > setAdditionalStyles []" ] ./packages/plugin-vite/tests/dev_server_test.ts => vite dev - css modules in _app/_layout/_error non-island component are injected ...----- post-test output end ----- ./packages/plugin-vite/tests/dev_server_test.ts => vite dev - css modules in _app/_layout/_error non-island component are injected ... FAILED (5s) ./packages/plugin-vite/tests/build_test.ts => vite build - tailwind _app ... ok (6s) ./packages/plugin-vite/tests/build_test.ts => vite build - partial island ... ok (1s) ------- post-test output -------

Hajime-san · 2026-04-26T11:13:37Z

+
+      if (ctx.#additionalStyles === null) {
+        ctx.#additionalStyles = css.slice();
+        return;


Yep, I added a FIXME comment below. I think it could be use Set instead of css: Array<string>.It seems that at least all the tests pass in the following branch.

Hajime-san/fresh@fix-ssr-css-modules...Hajime-san:fresh:perf-css

I'm planning it as a follow up PR.

Hajime-san · 2026-04-27T07:56:40Z

-            routeFileToName.set(route.filePath, name);
+            routeFileToName.set(toPosix(route.filePath), name);


seems to be same as https://github.com/denoland/fresh/pull/3781/changes#r3143377420

Hajime-san · 2026-04-27T07:56:53Z

-                const normalized = path.normalize(item.file);
+                const normalized = toPosix(path.normalize(item.file));


lunadogbot

Reviewed the diff — all four substantive points from the earlier review landed on 493f81a: replaceAll at server_snapshot.ts:536 with a regression test in build_test.ts:392 covering _app/_layout/_error; explicit appDef !== null guard at context.ts:326; the over-eager HMR re-walk was removed in 948dbb94; the includes() dedup at context.ts:205 carries a FIXME with a Set-based follow-up.

One thing worth keeping in mind: every route's virtual fresh-route-css::* module emits the same ["__FRESH_CSS_PLACEHOLDER__"] literal, so when multiple utility files hoist into a shared chunk, replaceAll writes the same combined info.css into each placeholder. That's correct today since setAdditionalStyles dedups on the render side — but if a future chunk-splitting strategy changes the "every placeholder in a chunk wants the same css list" assumption, this will need a rethink.

CI green on 493f81a.

lunadogbot · 2026-05-12T18:26:13Z

@bartlomieju this is ready to merge

bartlomieju · 2026-05-12T18:45:54Z

I feel like this placeholder situation should have better test coverage

bartlomieju

Thanks for the thorough fix — especially the dev + prod integration tests and the unit test for replaceFreshCssPlaceholders. The expanded placeholder replacement (all JS chunks containing FRESH_CSS_PLACEHOLDER rather than just _fresh-route___*) is the right shape for hoisted CSS, and the prod test asserting no leftover placeholders in any .mjs is a solid regression guard. Same for the toPosix normalization on both insert and lookup sides.

A couple of things worth double-checking before merging:

UiTree.app and UiTree.layouts[] changed shape (now { component, css } wrappers). It's behind getInternals/the internal symbol so blast radius should be small, but worth a grep across the monorepo to be sure nothing external reads internals.app as a component.
The semantics of setAdditionalStyles changed from "replace" to "accumulate + dedup". Looks correct given the new call sites and per-request #additionalStyles, but worth confirming no remaining caller depended on the old replace behavior.
_404: the diff threads css into newNotFoundCmd, but I didn't see the notFound pipeline pick it up explicitly. The /non_existent integration test passes so it presumably flows through renderRoute → setAdditionalStyles(ctx, route.css) — just calling it out to confirm.

A few smaller suggestions inline.

Move CssModuleNonIsland.tsx and CssModulesNonIsland.module.css from demo/components into the non_island_css_modules fixture directory, and update the _app.tsx import to match. The fixture is now self-contained.

Hajime-san added 8 commits April 25, 2026 14:24

add test

0dc5b17

works

c03d476

fix

cbc4256

simpler

69afe2e

revert

db93938

comment

1f42f05

comment

df3fcdb

try fix windows

f1934f0

Hajime-san force-pushed the fix-ssr-css-modules branch from fa5fa74 to f1934f0 Compare April 25, 2026 10:15

Hajime-san marked this pull request as draft April 25, 2026 11:29

bartlomieju reviewed Apr 25, 2026

View reviewed changes

Hajime-san added 6 commits April 26, 2026 11:17

add test fixture

5be11d3

replaceAll

bf4193a

Revert "try fix windows"

b6fe5ed

This reverts commit f1934f0.

lint

c54f479

rm

c5fd7e5

rm slop impl

a5d90ca

Hajime-san force-pushed the fix-ssr-css-modules branch 4 times, most recently from b069071 to 0395403 Compare April 26, 2026 07:58

normalize windows path

ba75ad3

Hajime-san force-pushed the fix-ssr-css-modules branch from 0395403 to ba75ad3 Compare April 26, 2026 08:14

Hajime-san added 2 commits April 26, 2026 19:54

review

881cd2a

review

9c1e62f

Hajime-san force-pushed the fix-ssr-css-modules branch from 1d16070 to 9c1e62f Compare April 26, 2026 11:43

review

814e3b5

Hajime-san commented Apr 26, 2026

View reviewed changes

Hajime-san marked this pull request as ready for review April 26, 2026 12:15

This comment was marked as outdated.

Sign in to view

Hajime-san requested a review from bartlomieju April 26, 2026 12:16

Hajime-san force-pushed the fix-ssr-css-modules branch 2 times, most recently from ef270d9 to 814e3b5 Compare April 27, 2026 03:55

Hajime-san added 4 commits April 27, 2026 15:04

rm wrong HMR

948dbb9

try windows

3770e6d

try windows

871e871

try windows

493f81a

Hajime-san commented Apr 27, 2026

View reviewed changes

This was referenced May 7, 2026

Tick 18: Quiet sweep tick (12h cron, new session) jayasri2805/SQL_Library_management#1

Closed

(removed) 6777777677578/insta-gc-bot-render#1

Closed

Disregard — created in error AnkeshM/Federated-Time-Series-Forecasting-main#1

Closed

lunadogbot approved these changes May 12, 2026

View reviewed changes

enhance test

7a597d9

bartlomieju reviewed May 26, 2026

View reviewed changes

Comment thread packages/fresh/src/context.ts Outdated

Comment thread packages/plugin-vite/tests/fixtures/non_island_css_modules/components/CssModuleNonIsland2.tsx

Hajime-san and others added 4 commits May 27, 2026 14:26

Merge branch 'main' into fix-ssr-css-modules

3dceedf

review for const loop

e75d5dd

mv fixture

9ce01e2

fix: colocate CssModuleNonIsland fixture files under tests/fixtures

f7f49ab

Move CssModuleNonIsland.tsx and CssModulesNonIsland.module.css from demo/components into the non_island_css_modules fixture directory, and update the _app.tsx import to match. The fixture is now self-contained.

bartlomieju approved these changes May 27, 2026

View reviewed changes

bartlomieju merged commit 3afaaf4 into freshframework:main May 27, 2026
7 checks passed

Hajime-san deleted the fix-ssr-css-modules branch May 27, 2026 06:56

Hajime-san mentioned this pull request Jun 3, 2026

How to style _layout.tsx? #3828

Open

		routeFileToName.set(route.filePath, name);
		routeFileToName.set(toPosix(route.filePath), name);

		const normalized = path.normalize(item.file);
		const normalized = toPosix(path.normalize(item.file));

Conversation

Hajime-san commented Apr 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bartlomieju left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hajime-san commented Apr 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lunadogbot left a comment

Choose a reason for hiding this comment

Uh oh!

lunadogbot commented May 12, 2026

Uh oh!

bartlomieju commented May 12, 2026

Uh oh!

bartlomieju left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Hajime-san commented Apr 25, 2026 •

edited

Loading

Hajime-san commented Apr 26, 2026 •

edited

Loading