-
Notifications
You must be signed in to change notification settings - Fork 0
fix(config,release-assets): stop hosted config and CSS scan failing valid projects #3402
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
Changes from all commits
d423779
aea7aaf
324abbf
ab515bc
487bac2
1ef7df6
1228d57
f55a350
47f00f9
02232f6
5e12106
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,14 +25,39 @@ import { isWithinDirectory, normalizePath } from "#veryfront/utils/path-utils.ts | |
| export const CSS_IMPORTING_SOURCE_EXTENSIONS = [".tsx", ".jsx", ".mdx", ".ts", ".js"]; | ||
|
|
||
| /** | ||
| * Static ESM import statements whose specifier ends in `.css`: | ||
| * ESM imports whose specifier ends in `.css`: | ||
| * import "./styles.css"; | ||
| * import styles from "./button.module.css"; | ||
| * `[^'";]*` keeps the match from crossing statement boundaries. | ||
| * import("./theme.css") | ||
| * | ||
| * Dynamic imports are matched on purpose, despite this once being described as | ||
| * static-only. `import("./theme.css")` loads that stylesheet at runtime, so | ||
| * leaving it out means the compiled stylesheet is missing CSS the page actually | ||
| * uses. A dynamic specifier pointing at a file that does not exist is a broken | ||
| * reference, not a false positive -- exactly as a static one would be. | ||
| * `[^'";]*` keeps the match from crossing statement boundaries, and `\bimport\b` | ||
| * keeps identifiers that merely contain the word out of it -- without it, | ||
| * `const important = "./styles.css"` reads as an import. That matters more here | ||
| * than it looks: release-asset builds turn a bogus specifier into a fatal | ||
| * coverage gap, so a false positive fails the release. | ||
| */ | ||
| const CSS_IMPORT_RE = /import[^'";]*['"]([^'"]+\.css)['"]/g; | ||
| const CSS_IMPORT_RE = /\bimport\b(?!\s*\.)[^'";]*['"]([^'"]+\.css)['"]/g; | ||
|
|
||
| /** Extract the raw specifiers of all static CSS imports in a source file. */ | ||
| /** | ||
| * Extract the raw specifiers of all CSS imports in a source file. | ||
| * | ||
| * Deliberately loose, per this module's contract: over-matching is harmless | ||
| * because unresolvable specifiers are skipped downstream. A commented-out or | ||
| * quoted `import "./x.css"` will be reported, and that is fine. | ||
| * | ||
| * An earlier revision blanked comments, template literals and fenced blocks | ||
| * before matching, because the release-asset build had made this function's | ||
| * output fatal. That was the wrong layer to fix it: telling code from prose | ||
| * with a regex kept finding new holes, and worse, an unpaired `/*` or backtick | ||
| * blanked across intervening real code and silently dropped a genuine import -- | ||
| * trading a loud failure for a page shipped without its stylesheet. The build | ||
| * no longer gaps on what it cannot resolve, so the looseness costs nothing. | ||
|
Comment on lines
+49
to
+59
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. 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift Do not merge resolvable false positives. A commented or quoted Restore code-aware import detection. Add a release-build regression with a commented or quoted import and an existing stylesheet. Ensure the stylesheet is not merged. As per coding guidelines, behavior changes must have focused tests. 🤖 Prompt for AI AgentsSource: Coding guidelines
Contributor
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. Valid, and deliberately left open — this one is not obsolete and is not fixed. You found the gap in my reasoning. I justified the looseness with "unresolvable specifiers are skipped downstream, so over-matching is harmless", and that holds only for unresolvable ones. A commented or quoted Weighing it against what it replaced: shipping extra CSS is a smaller failure than refusing every release for a project with a JSX component, which is where The real fix is the one kwakayama independently proposed: genuine CSS-import detection belongs on the resolved module graph ( Out of scope for this PR, which is an incident fix. Tracking it as the follow-up rather than attempting it at the end of a long session — the last four rounds on this file each produced a new hole, and this one deserves to be done deliberately. 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.
The current text scan can merge a stylesheet from a comment or quoted string when that stylesheet exists. Deferring the fix is reasonable for this incident-focused PR. Moving CSS import detection to 🐇🔍 You are interacting with an AI system. |
||
| */ | ||
| export function extractCssImportSpecifiers(source: string): string[] { | ||
| const specifiers: string[] = []; | ||
| for (const match of source.matchAll(CSS_IMPORT_RE)) { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.