-
Notifications
You must be signed in to change notification settings - Fork 751
fix: CSS modules not working in _app/_layout/_error and across routes in non-island components #3781
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
base: main
Are you sure you want to change the base?
fix: CSS modules not working in _app/_layout/_error and across routes in non-island components #3781
Changes from all commits
0dc5b17
c03d476
cbc4256
69afe2e
db93938
1f42f05
df3fcdb
f1934f0
5be11d3
bf4193a
b6fe5ed
c54f479
c5fd7e5
a5d90ca
ba75ad3
881cd2a
9c1e62f
814e3b5
948dbb9
3770e6d
871e871
493f81a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -98,8 +98,11 @@ export type ServerIslandRegistry = Map<ComponentType, Island>; | |
| export const internals: unique symbol = Symbol("fresh_internal"); | ||
|
|
||
| export interface UiTree<Data, State> { | ||
| app: AnyComponent<PageProps<Data, State>> | null; | ||
| layouts: ComponentDef<Data, State>[]; | ||
| app: { | ||
| component: AnyComponent<PageProps<Data, State>>; | ||
| css: string[] | null; | ||
| } | null; | ||
| layouts: (ComponentDef<Data, State> & { css: string[] | null })[]; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -109,7 +112,10 @@ export type FreshContext<State = unknown> = Context<State>; | |
|
|
||
| export let getBuildCache: <T>(ctx: Context<T>) => BuildCache<T>; | ||
| export let getInternals: <T>(ctx: Context<T>) => UiTree<unknown, T>; | ||
| export let setAdditionalStyles: <T>(ctx: Context<T>, css: string[]) => void; | ||
| export let setAdditionalStyles: <T>( | ||
| ctx: Context<T>, | ||
| css: string[] | null | undefined, | ||
| ) => void; | ||
|
|
||
| /** | ||
| * The context passed to every middleware. It is unique for every request. | ||
|
|
@@ -182,8 +188,25 @@ export class Context<State> { | |
| // deno-lint-ignore no-explicit-any | ||
| getInternals = <T>(ctx: Context<T>) => ctx.#internal as any; | ||
| getBuildCache = <T>(ctx: Context<T>) => ctx.#buildCache; | ||
| setAdditionalStyles = <T>(ctx: Context<T>, css: string[]) => | ||
| ctx.#additionalStyles = css; | ||
| setAdditionalStyles = <T>( | ||
| ctx: Context<T>, | ||
| css: string[] | null | undefined, | ||
| ) => { | ||
| if (css == null) return; | ||
|
|
||
| if (ctx.#additionalStyles === null) { | ||
| ctx.#additionalStyles = css.slice(); | ||
| return; | ||
| } | ||
|
|
||
| for (let i = 0; i < css.length; i++) { | ||
| const href = css[i]; | ||
| // FIXME: consider to use `Set` instead of `css: string[]` for entire codebase | ||
| if (!ctx.#additionalStyles.includes(href)) { | ||
| ctx.#additionalStyles.push(href); | ||
| } | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| constructor( | ||
|
|
@@ -283,6 +306,7 @@ export class Context<State> { | |
| props.Component = () => child; | ||
|
|
||
| const def = defs[i]; | ||
| setAdditionalStyles(this, def.css); | ||
|
|
||
| const result = await renderRouteComponent(this, def, () => child); | ||
| if (result instanceof Response) { | ||
|
|
@@ -298,16 +322,20 @@ export class Context<State> { | |
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: if (appDef !== null) {
setAdditionalStyles(this, appDef.css);
}Especially since the |
||
| let hasApp = true; | ||
|
|
||
| if (isAsyncAnyComponent(appDef)) { | ||
| if (appDef !== null) { | ||
| setAdditionalStyles(this, appDef.css); | ||
| } | ||
|
|
||
| if (appDef !== null && isAsyncAnyComponent(appDef.component)) { | ||
| props.Component = () => appChild; | ||
| const result = await renderAsyncAnyComponent(appDef, props); | ||
| const result = await renderAsyncAnyComponent(appDef.component, props); | ||
| if (result instanceof Response) { | ||
| return result; | ||
| } | ||
|
|
||
| appVNode = result; | ||
| } else if (appDef !== null) { | ||
| appVNode = h(appDef, { | ||
| appVNode = h(appDef.component, { | ||
| Component: () => appChild, | ||
| config: this.config, | ||
| data: null, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| import { type FsAdapter, fsAdapter } from "../fs.ts"; | ||
| import type { WalkEntry } from "@std/fs/walk"; | ||
| import type { FsRouteFileNoMod } from "./dev_build_cache.ts"; | ||
| import { type FsRouteFileNoMod, toPosix } from "./dev_build_cache.ts"; | ||
| import * as path from "@std/path"; | ||
| import { pathToPattern } from "../router.ts"; | ||
| import { CommandType } from "../commands.ts"; | ||
|
|
@@ -86,7 +86,7 @@ export async function crawlRouteDir<State>( | |
|
|
||
| files.push({ | ||
| id, | ||
| filePath: entry.path, | ||
| filePath: toPosix(entry.path), | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:8debug 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 ------- |
||
| type, | ||
| pattern, | ||
| routePattern, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| import styles from "./CssModulesNonIsland2.module.css"; | ||
|
|
||
| export function CssModulesNonIsland2() { | ||
| return ( | ||
| <div class="blue"> | ||
| <h1 class={styles.root}>blue text</h1> | ||
| </div> | ||
| ); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| import styles from "./CssModulesNonIsland3.module.css"; | ||
|
|
||
| export function CssModulesNonIsland3() { | ||
| return ( | ||
| <div class="red"> | ||
| <h1 class={styles.root}>red text</h1> | ||
| </div> | ||
| ); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| .root { | ||
| color: blue; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| .root { | ||
| color: red; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor:
includes()is O(n) per insertion. For the small CSS lists in practice this is fine, but aSetwould be more idiomatic if you ever want to tighten this up. Low priority.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I added a FIXME comment below. I think it could be use
Setinstead ofcss: 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.