From 72506f3b4a5cbb7acba250aeceaab27755742421 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20=C3=81ngel?= Date: Sun, 17 May 2026 17:35:31 -0400 Subject: [PATCH] =?UTF-8?q?fix(core):=20address=20#922=20review=20?= =?UTF-8?q?=E2=80=94=20posix=20paths,=20diamond=20imports,=20comment=20saf?= =?UTF-8?q?ety?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes from review #4306329284 on hf#922: - Normalize path.relative() output with .split(sep).join("/") so rebased url() paths use forward slashes on Windows, matching the posix-path convention in rewriteSubCompPaths.ts. - Return empty string (not the original @import statement) when the visited set detects a diamond import. Previously the stale @import leaked through and caused a 404 after bundling. - Strip CSS block comments before @import matching so commented-out imports (/* @import url(...) */) are not resolved. Comments are restored after processing via placeholder substitution. --- .../core/src/compiler/htmlBundler.test.ts | 41 ++++++++++++++++++ packages/core/src/compiler/htmlBundler.ts | 42 +++++++++++++++---- 2 files changed, 75 insertions(+), 8 deletions(-) diff --git a/packages/core/src/compiler/htmlBundler.test.ts b/packages/core/src/compiler/htmlBundler.test.ts index 1e2ddf89d..2dbbd4e35 100644 --- a/packages/core/src/compiler/htmlBundler.test.ts +++ b/packages/core/src/compiler/htmlBundler.test.ts @@ -898,4 +898,45 @@ describe("bundleToSingleHtml", () => { expect(bundled).toContain("url('styles/sprite.png?v=2#section')"); }); + + it("deduplicates diamond @import (same file imported by two parents)", async () => { + const dir = makeTempProject({ + "index.html": ` + + +
+ +`, + "styles/main.css": `@import url('./a.css');\n@import url('./b.css');`, + "styles/a.css": `@import url('./shared.css');\n.a { color: red; }`, + "styles/b.css": `@import url('./shared.css');\n.b { color: blue; }`, + "styles/shared.css": `:root { --shared: 1; }`, + }); + + const bundled = await bundleToSingleHtml(dir); + + const sharedCount = (bundled.match(/--shared: 1/g) || []).length; + expect(sharedCount).toBe(1); + expect(bundled).toContain(".a { color: red; }"); + expect(bundled).toContain(".b { color: blue; }"); + expect(bundled).not.toContain("@import"); + }); + + it("does not resolve @import inside CSS comments", async () => { + const dir = makeTempProject({ + "index.html": ` + + +
+ +`, + "app.css": `/* @import url('./old.css'); */\nbody { margin: 0; }`, + "old.css": `.old { display: none; }`, + }); + + const bundled = await bundleToSingleHtml(dir); + + expect(bundled).toContain("/* @import url('./old.css'); */"); + expect(bundled).not.toContain(".old { display: none; }"); + }); }); diff --git a/packages/core/src/compiler/htmlBundler.ts b/packages/core/src/compiler/htmlBundler.ts index cf45b8b0d..a17050dc9 100644 --- a/packages/core/src/compiler/htmlBundler.ts +++ b/packages/core/src/compiler/htmlBundler.ts @@ -92,6 +92,29 @@ const CSS_IMPORT_RE = const REBASE_URL_RE = /\burl\(\s*(["']?)([^)"']+)\1\s*\)/g; +const CSS_COMMENT_RE = /\/\*[\s\S]*?\*\//g; + +function withCommentsStripped( + css: string, + fn: (stripped: string) => T, +): { result: T; restore: (s: string) => string } { + const comments: string[] = []; + const stripped = css.replace(CSS_COMMENT_RE, (m) => { + const idx = comments.length; + comments.push(m); + return `/*__hf_c${idx}__*/`; + }); + const result = fn(stripped); + const restore = (s: string) => { + let out = s; + for (let i = 0; i < comments.length; i++) { + out = out.replace(`/*__hf_c${i}__*/`, comments[i]!); + } + return out; + }; + return { result, restore }; +} + function rebaseCssUrls(css: string, cssFileDir: string, projectDir: string): string { const resolvedRoot = resolve(projectDir); const resolvedDir = resolve(cssFileDir); @@ -101,7 +124,7 @@ function rebaseCssUrls(css: string, cssFileDir: string, projectDir: string): str const { basePath, suffix } = splitUrlSuffix(urlValue.trim()); if (!basePath) return full; const absolutePath = resolve(resolvedDir, basePath); - const rebased = relative(resolvedRoot, absolutePath); + const rebased = relative(resolvedRoot, absolutePath).split(sep).join("/"); if (rebased === basePath) return full; return `url(${quote || ""}${rebased}${suffix}${quote || ""})`; }); @@ -113,29 +136,32 @@ function inlineCssFile( projectDir: string, visited: Set = new Set(), ): string { - const placeholders: string[] = []; - const withPlaceholders = css.replace( + const { result: strippedCss, restore: restoreComments } = withCommentsStripped(css, (s) => s); + const importPlaceholders: string[] = []; + const withPlaceholders = strippedCss.replace( CSS_IMPORT_RE, (full, _q1, urlPath, _q2, barePath, mediaQuery) => { const importPath = urlPath ?? barePath; if (!importPath || !isRelativeUrl(importPath)) return full; const resolved = resolve(cssFileDir, importPath); const normalizedBase = resolve(projectDir) + sep; - if (!resolved.startsWith(normalizedBase) || visited.has(resolved)) return full; + if (!resolved.startsWith(normalizedBase)) return full; + if (visited.has(resolved)) return ""; const content = safeReadFile(resolved); if (content == null) return full; visited.add(resolved); const inlined = inlineCssFile(content, dirname(resolved), projectDir, visited); const trimmedMedia = (mediaQuery || "").trim(); const block = trimmedMedia ? `@media ${trimmedMedia} {\n${inlined}\n}\n` : inlined + "\n"; - const idx = placeholders.length; - placeholders.push(block); + const idx = importPlaceholders.length; + importPlaceholders.push(block); return `/*__hf_import_${idx}__*/`; }, ); let rebased = rebaseCssUrls(withPlaceholders, cssFileDir, projectDir); - for (let i = 0; i < placeholders.length; i++) { - rebased = rebased.replace(`/*__hf_import_${i}__*/`, placeholders[i]!); + rebased = restoreComments(rebased); + for (let i = 0; i < importPlaceholders.length; i++) { + rebased = rebased.replace(`/*__hf_import_${i}__*/`, importPlaceholders[i]!); } return rebased; }