Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/wicked-laws-bake.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@solidjs/start": patch
---

Fixed virtual module CSS not being collected in vite dev.
6 changes: 6 additions & 0 deletions apps/fixtures/css/src/components/test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,12 @@ export const CommonTests = (props: { routeModuleClass?: string }) => (
class={props.routeModuleClass}
integration="module"
/>
<Test
component="Route"
file="virtual:virtualModule.css"
class="virtualCss"
comment="CSS from virtual module (virtualCssPlugin.ts)."
/>
<Test
component="Route"
file="url.css"
Expand Down
1 change: 1 addition & 0 deletions apps/fixtures/css/src/routes/index.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { createAsync, query } from "@solidjs/router";
import { lazy, Show } from "solid-js";
import "virtual:virtualModule.css";
import Layout from "../components/layout";
import { CommonTests } from "../components/test";
import notRenderedInlineCSS from "../styles/notRendered.css?url";
Expand Down
18 changes: 18 additions & 0 deletions apps/fixtures/css/src/virtualCssPlugin.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { Plugin } from "vite";

const id = "virtual:virtualModule.css";
const resolvedId = "\0" + id;

const virtualCSS = () =>
({
name: "css-fixture-virtual-css",
resolveId(source) {
if (source === id) return resolvedId;
},
load(id) {
if (id.startsWith(resolvedId))
return `.virtualCss { background-color: var(--color-success); }`;
},
}) satisfies Plugin;

export default virtualCSS;
3 changes: 2 additions & 1 deletion apps/fixtures/css/vite.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ import tailwindcss from "@tailwindcss/vite";
import { defineConfig } from "vite";
import { nitroV2Plugin } from "../../../packages/start-nitro-v2-vite-plugin/src";
import { solidStart } from "../../../packages/start/src/config";
import virtualCSS from "./src/virtualCssPlugin";

export default defineConfig({
plugins: [solidStart(), nitroV2Plugin(), tailwindcss()],
plugins: [virtualCSS(), solidStart(), nitroV2Plugin(), tailwindcss()],
});
5 changes: 3 additions & 2 deletions packages/start/src/config/manifest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { type PluginOption, type ViteDevServer } from "vite";
import { findStylesInModuleGraph } from "../server/collect-styles.ts";
import { VIRTUAL_MODULES } from "./constants.ts";
import { type SolidStartOptions } from "./index.ts";
import { wrapId } from "./vite-utils.ts";

export function manifest(start: SolidStartOptions): PluginOption {
let devServer: ViteDevServer = undefined!;
Expand Down Expand Up @@ -69,10 +70,10 @@ export function manifest(start: SolidStartOptions): PluginOption {
tag: "style",
attrs: {
type: "text/css",
"data-vite-dev-id": "${key}",
"data-vite-dev-id": "${wrapId(key)}",
"data-vite-ref": "0",
},
children: () => import("${value}?inline").then(mod => mod.default),
children: () => import("${wrapId(value)}?inline").then(mod => mod.default),
}`,
);

Expand Down
10 changes: 7 additions & 3 deletions packages/start/src/config/vite-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ export const FS_PREFIX = `/@fs/`;
export const VALID_ID_PREFIX = `/@id/`;

export const NULL_BYTE_PLACEHOLDER = `__x00__`;
const NULL_BYTE_REGEX = /^\0/;

export function normalizeResolvedIdToUrl(
environment: DevEnvironment,
Expand Down Expand Up @@ -52,10 +53,13 @@ export function normalizeResolvedIdToUrl(
return url;
}

/**
* Inspired by:
* https://github.com/withastro/astro/blob/fddde5fad81007795eb263c7fd0cea096b8e2cba/packages/astro/src/core/util.ts#L115
* https://github.com/vitejs/vite/blob/130e7181a55c524383c63bbfb1749d0ff7185cad/packages/vite/src/shared/utils.ts#L11
*/
export function wrapId(id: string): string {
return id.startsWith(VALID_ID_PREFIX)
? id
: VALID_ID_PREFIX + id.replace("\0", NULL_BYTE_PLACEHOLDER);
return id.replace(NULL_BYTE_REGEX, `${VALID_ID_PREFIX}${NULL_BYTE_PLACEHOLDER}`);
}

export function unwrapId(id: string): string {
Expand Down
2 changes: 2 additions & 0 deletions packages/start/src/server/StartServer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import App from "solid-start:app";

import { ErrorBoundary, TopErrorBoundary } from "../shared/ErrorBoundary.tsx";
import { useAssets } from "./assets/index.ts";
import PatchVirtualDevStyles from "./assets/PatchVirtualDevStyles.tsx";
import { getSsrManifest } from "./manifest/ssr-manifest.ts";
import type { DocumentComponentProps, PageEvent } from "./types.ts";

Expand All @@ -29,6 +30,7 @@ export function StartServer(props: { document: Component<DocumentComponentProps>
assets={<HydrationScript />}
scripts={
<>
<PatchVirtualDevStyles nonce={nonce} />
<script
type="module"
nonce={nonce}
Expand Down
31 changes: 31 additions & 0 deletions packages/start/src/server/assets/PatchVirtualDevStyles.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/**
* Patches the data-vite-dev-id attribute for style tags of virtual modules
*
* Per Vite's convention, virtual module ids are prefixed with \0 (null byte):
* https://vite.dev/guide/api-plugin#virtual-modules-convention
*
* However this null byte cannot be server rendered properly.
* Vite client runtime then fails to find style's with wrong null bytes,
* and instead inserts duplicate style's.
*
* This patch replaces the serializable /@id/__x00__ with the proper null byte,
* and has to run before Vite's client runtime:
* https://github.com/vitejs/vite/blob/130e7181a55c524383c63bbfb1749d0ff7185cad/packages/vite/src/client/client.ts#L529
*
* TODO: This should be solved in Vite directly!
*/
const patch = function () {
document.querySelectorAll<HTMLElement>("style[data-vite-dev-id]").forEach(function (el) {
el.setAttribute("data-vite-dev-id", el.dataset.viteDevId!.replace("/@id/__x00__", "\0"));
});
};

const serializedPatch = `(${patch.toString()})();`;

const PatchVirtualDevStyles = (props: { nonce?: string }) => {
if (!import.meta.env.PROD) {
return <script nonce={props.nonce} innerHTML={serializedPatch} />;
}
};

export default PatchVirtualDevStyles;
35 changes: 9 additions & 26 deletions packages/start/src/server/collect-styles.ts
Original file line number Diff line number Diff line change
@@ -1,40 +1,23 @@
import path from "node:path";
import { resolve } from "pathe";
import type { DevEnvironment, EnvironmentModuleNode } from "vite";

async function getViteModuleNode(vite: DevEnvironment, file: string) {
let nodePath = file;
let node = vite.moduleGraph.getModuleById(file);

if (!node) {
const resolvedId = await vite.pluginContainer.resolveId(file, undefined);
if (!resolvedId) return;

nodePath = resolvedId.id;
node = vite.moduleGraph.getModuleById(file);
}

if (!node) {
nodePath = resolve(nodePath);
node = await vite.moduleGraph.getModuleByUrl(file);
}

if (!node) {
await vite.moduleGraph.ensureEntryFromUrl(nodePath, false);
node = vite.moduleGraph.getModuleById(nodePath);
}

return node;
async function getViteModuleNode(vite: DevEnvironment, file: string, importer?: string) {
try {
const res = await vite.fetchModule(file, importer);
if (!("id" in res)) return;
return vite.moduleGraph.getModuleById(res.id);
Copy link
Member

Choose a reason for hiding this comment

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

much cleaner! 🥇

but this is removing the fallback vite.fetchModuleByUrl and relying only in the moduleid- is this not needed or am I missing something else? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

relying only in the moduleid

The new line const res = await vite.fetchModule(file, importer); resolves the raw file url. But it's result doesn't include all fields from the module, that's the reason why I have to also call vite.moduleGraph.getModuleById on the later line. The logic here is:

  1. Resolve raw file url to module id via vite.fetchModule
  2. Get missing module fields via vite.moduleGraph.getModuleById

removing the fallback vite.fetchModuleByUrl

fetchModuleByUrl - is this a typo 🤔? Afaik this API does not exist, but I assume you mean getModuleByUrl / ensureEntryFromUrl?

So to "quickly" comment my thoughts about those:

Removed ensureEntryFromUrl

The new vite.fetchModule call internally already calls ensureEntryFromUrl (https://github.com/vitejs/vite/blob/964c718a382ff46ec1f906d7d6bc3f135a6dcd3f/packages/vite/src/node/ssr/fetchModule.ts#L83), so we don't have to do that manually anymore.

Removed getModuleByUrl

nodePath = resolve(nodePath);
node = await vite.moduleGraph.getModuleByUrl(file);
  • This old logic was already broken, because getModuleByUrl was called with file instead of nodePath 🙈.
  • But assuming that the old logic somehow was correct: getModuleByUrl basically is a small subset of ensureEntryFromUrl.

Removed getModuleById

  • The old logic also partially already was broken to begin with
  • But assuming that the old logic somehow was correct: ensureEntryFromUrl internally calls _resolveUrl and _resolveUrl uses _resolveId to get the id from the url.

Extra words: To be completely honest, deleting all that old logic and replacing it with the simple two lines didn't exactly feel good, especially because we do not have lot's of tests around this. I tried my best to test the new logic, but there's certainly the chance that I missed something. So there is the risk that with the new logic some unexpected cases might pop up where CSS is not server rendered during the alpha. This however gives us the opportunity to then include those cases in the CSS fixture and bring back the few actually needed parts of the old logic, if necessary. What do you think of this plan @atilafassina (and @Brendonovich)?

Copy link
Member

Choose a reason for hiding this comment

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

thanks for the explanation!!

I agree with your reasoning: removing what looks wrong/unnecessary/redundant and watching for edge cases

} catch (err) {}
}

async function findModuleDependencies(
vite: DevEnvironment,
file: string,
deps: Set<EnvironmentModuleNode>,
crawledFiles = new Set<string>(),
importer?: string,
) {
crawledFiles.add(file);
const module = await getViteModuleNode(vite, file);
const module = await getViteModuleNode(vite, file, importer);
if (!module?.id || deps.has(module)) return;

deps.add(module);
Expand All @@ -53,7 +36,7 @@ async function findModuleDependencies(
if (crawledFiles.has(dep)) {
continue;
}
await findModuleDependencies(vite, dep, deps, crawledFiles);
await findModuleDependencies(vite, dep, deps, crawledFiles, module.id);
}
}

Expand Down
2 changes: 2 additions & 0 deletions packages/start/src/server/spa/StartServer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { getSsrManifest } from "../manifest/ssr-manifest.ts";

import { TopErrorBoundary } from "../../shared/ErrorBoundary.tsx";
import { useAssets } from "../assets/index.ts";
import PatchVirtualDevStyles from "../assets/PatchVirtualDevStyles.tsx";
import type { DocumentComponentProps, PageEvent } from "../types.ts";

const docType = ssr("<!DOCTYPE html>");
Expand All @@ -27,6 +28,7 @@ export function StartServer(props: { document: Component<DocumentComponentProps>
<props.document
scripts={
<>
<PatchVirtualDevStyles nonce={nonce} />
<script
type="module"
src={getSsrManifest("client").path(import.meta.env.START_CLIENT_ENTRY)}
Expand Down
Loading