fix(vite): clear a component's styles when it loses its last one - #461
Conversation
A component that lost its last style kept the old CSS applied until a
full reload. The generated HMR module opens with `...ClassName.ɵcmp`, so
every key it does not emit keeps its previous value — and `styles` was
omitted for every "no styles" answer.
`styles` is now three-valued, end to end:
non-empty these are the component's styles
[] it definitively has none — emit `styles: []` and clear
null unknown — omit the key, whatever it has survives
Four places collapsed the first two into one value. `index.ts:734` turned
an empty result into null before the call; `lib.rs:555` merged `None` and
`Some([])` with `unwrap_or_default`; `lib.rs:561`/`:577` mapped an empty
list back to `None`; and `update_module.rs:182` guarded the emit a second
time. Fixing only the generator changes nothing, because nothing ever
handed it a `Some([])`.
`null` was also overloaded in the plugin. Clearing on an unreadable
decorator would wipe live CSS, so the distinction that matters is whether
the answer is KNOWN, not whether it is empty. `readStyles` now reports a
failed read instead of swallowing it in a `catch`: a stylesheet read
successfully but holding nothing is definitively styleless and clears,
while one that could not be read at all stays unknown and is left alone.
An editor's atomic write leaves exactly that window. The file-level
fallback branch keeps its old semantics and can never clear.
Verified against the pinned Angular v22.0.0: `ɵɵreplaceMetadata` →
`recreateLView` → `destroyLView` → `sharedStylesHost.removeStyles`, which
is ref-counted per CSS string. Today the removed and re-added strings are
identical, so the count dips and returns and the `<style>` element never
leaves; with `styles: []` nothing is re-added, the count reaches zero and
`element.remove()` runs.
Tests cover both directions: the three clearing transitions, and the two
that must NOT clear (an unreadable stylesheet, an unreadable style
field). One existing assertion changed deliberately —
`hmr-hot-update.test.ts` asserted `not.toContain('styles:')` for a class
declaring none, which encoded the old behavior; it now asserts the
explicit empty array, keeping its original intent. A Playwright spec
proves the user-visible effect: the border reverts and no `full-reload`
crosses the wire.
Closes #457
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01665HouXD8imyMphWe1k7Dx
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_92ef0746-579d-4c83-a7fd-e7def1961d66) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6e339d331
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…tyles The fallback branch runs when the class's OWN decorator could not be read, and it falls back to the file-level union — which can belong to a sibling. Its comment already promised it never clears. The code stopped keeping that promise once this PR made any non-null array definitive. A stylesheet that reads fine but holds nothing yields `['']`. That is one entry, so `contents.length > 0` passed it on; the binding then dropped the whitespace-only entry and emitted `styles: []`, wiping CSS from a decorator nobody on this path could read — `styles: STYLE_CONST`, say, which the Rust extractor folds and the text scan cannot see. Measured: `['']` and `[' ']` both produced `styles: []`, while `null` omitted the key. Before this PR the same input was omitted, so the regression is this PR's. The whitespace filter now runs BEFORE the null check here, unlike the class-level branch above where an empty result is a real answer. A partial read on this branch still presents the successfully-read subset as the whole style set. That is unchanged from main — the old `readStyles` swallowed failures in a `catch` and returned whatever it got — and belongs to the file-level fallback contamination tracked in #456, not here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01665HouXD8imyMphWe1k7Dx
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_54f4e367-7381-4795-9e77-d5eacbd36b6e) |
|
The medium finding is confirmed and fixed in 38812d0. The second observation is real but pre-existing, and I am leaving it. Confirmed: the fallback branch could clearMeasured at the binding on
The severity is right for the reason you gave: this branch only runs when Fix: const fallback = await readStyles(styleUrls)
const usable = fallback.contents.filter((style) => style.trim().length > 0)
styles = usable.length > 0 ? usable : nullThe whitespace filter now runs BEFORE the null check on this branch, unlike the class-level branch above, where an empty result IS a definitive answer. That asymmetry is the whole point and is now commented. Two regressions added, both red-checked. Each is one file with two classes: the requested class declares The three clearing cases still clear. Suite: 408 -> 410. Declined: the partial-read replacement
True, and unchanged from } catch {
// Style file not found, continue without this style
}
...
return styleContents.length > 0 ? styleContents : nullwith a bare I am not adding a On the framingThe recommendation to "keep |
A component with two external stylesheets, one of them permanently unreadable, stopped receiving style updates entirely. Editing the HEALTHY stylesheet dispatched HMR, but `external.complete` was false, so `styles` stayed null and the module omitted the key. The edit was lost, and the pending slot is consumed either way, so nothing retried it. `complete` guards exactly ONE thing: turning an unknown answer into a definitive `[]` that wipes live CSS. It is not a reason to throw away content that WAS read. main always delivered the partial read — the old `readStyles` swallowed failures in a `catch` and returned whatever it got — and this PR broke that. complete -> merged, may be [], may clear (new) incomplete, merged non-[] -> merged (= main) incomplete, merged == [] -> null, key omitted (= main) Verified by transcribing both revisions' formulas and sweeping all 105 combinations of inline shape against read outcomes: 21 differ from main, and all 21 are the intended "complete read, empty result -> styles: []". Zero other differences. The fallback branch below is unchanged and already correct. It ignores `complete` and applies the same never-clear rule, because a successful read of the file-level union still says nothing definitive about a class whose own decorator could not be read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01665HouXD8imyMphWe1k7Dx
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_ad8325f4-1271-4433-a725-37144f55e5dd) |
…462) * fix(vite): resolve HMR resources from the extractor, not a text scan The `@ng/component` endpoint read a class's own `templateUrl` / `template` / `styleUrl(s)` / `styles` by scanning decorator TEXT. The Rust extractor folds same-file constants and interpolates template literals; the scan cannot. Every shape it could not read fell back to the FILE-LEVEL union, which in a multi-component file served a class its siblings' template and stylesheets. const DIR = './themes' @component({ styleUrls: [`${DIR}/a.css`, SHARED_STYLE] }) The endpoint now asks the extractor. `extract_component_metadata_sync` was already written and correct but carried no `#[napi]`, so it was not on the JS surface; adding the attribute is the whole Rust change. That answer is definitive, because it is the SAME one the compiler uses. `transform.rs:2557` and the extractor both call `extract_component_metadata` with the same `collect_string_consts` table. The transform's only extra step, `resolve_styles`, turns URLs into content and never re-derives the URL list: compiled styles = metadata.styles ++ concat(content(u) for u in style_urls) └── inline, first ──┘ Eight fixtures were compared against the real compile and all matched, including the one that looks like a partial read: `[OK, IMPORTED, './lit.css']` resolves to two entries in BOTH. The dropped import is genuinely not a stylesheet this component gets, so serving two is exact, not partial. A class MISSING from the metadata is equally definitive — measured, the compiler skips it too, leaving the decorator intact with no `ɵcmp`. So both fallbacks are deleted rather than narrowed. This retires the `unreadable` state, which existed only because the text scan disagreed with Rust. What does NOT change is `readStyles`, its `complete` flag, and the three-valued `styles` argument from #457/#461: those guard a resolved file that cannot be READ, which is a filesystem question and still real. Three existing assertions flipped, deliberately. `styles: SOME_ARRAY_CONST` asserted no `styles:` key on the theory that Rust folded the constant. It does not — OXC folds string-valued consts, not array-valued ones, so that component's `ɵcmp` never had a `styles` property. `styles: []` there clears nothing. Verified against the compiler output, not reasoned. Closes #456 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01665HouXD8imyMphWe1k7Dx * refactor(vite): delete the decorator locators the endpoint no longer uses The previous commit moved HMR resource resolution to the extractor, which left the per-class text locators reachable only from their own tests. Gone from the plugin: `extractTemplateUrlFor`, `extractClassStylesFor`, `extractInlineTemplate`, `extractInlineStyles`, and the two caches the last pair existed to fill — `inlineTemplateCache` and `inlineStylesCache`, both of which were written, pruned, refreshed on hot update, and never read by anything, from before this PR. Gone from the scanner: the `*For` locator family, `readStringLiterals`, `StringLiteralsRead`, `ClassStyleFields`, `locateStyleFieldsFor`, `hasUnreadableKey`, `hasInterpolation`. Its exports drop from 15 to 6. `stripComponentMetadata` and its closure stay untouched. It decides full reload versus hot update, so it still parses decorator text and still has to get comments, decoys and escapes right. Most of what looked dead was not. Nine of ten candidate helpers turned out live via `locateStylesInArgs` / `locateTemplateInArgs` -> `locateFieldInsideArgs` -> `findFieldInArgs`; only `hasInterpolation` was genuinely unreachable. `FieldValue` stays as `findFieldInArgs`'s return type, with its `export` dropped. So the tests were re-pointed rather than dropped: of 206, 22 are unchanged, 78 keep their title with the call swapped to a surviving locator, 26 are renamed because the old title named a concept that is gone, and 76 are deleted — the `readStringLiterals` block and the url-locator blocks, which test functions that no longer exist. Two new cases cover strip-path behaviour the salvage exposed: a `]` inside a comment must not close the array early, and an array holding only a comment must still strip to `[]`. Behaviour is proven unchanged beyond the e2e suite: `stripComponentMetadata` was run against both the old and new scanner over 744 generated sources — 62 decorator shapes by 12 file wrappers, including phantom decorators in comments and strings, CRLF, Unicode class names, malformed escapes, spreads and elisions. Zero differences. 343 unit tests pass, e2e stays at 37. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01665HouXD8imyMphWe1k7Dx * fix(vite): clear styles only from the source the component compiled with The endpoint re-parses `resolvedId` from disk, but the component was compiled from the `code` Vite handed `transform`. Those are different byte streams whenever another plugin exposes a `load` hook or a pre-ordered `transform`, and — with no third-party plugin at all — whenever `fileReplacements` points `actualId` at a different file. On a disk source the compiler never saw, every `styles` shape the extractor cannot fold resolves to nothing: an array constant, an imported one, a `.concat(...)`. Reading that as definitive emitted `styles: []` and wiped CSS the running component genuinely had — from a template edit that never touched the styles. Measured on a real dev server, disk holding `styles: STYLE_ARRAY` with an upstream `load` expanding it, editing only the external `.html`: this branch styles: [], <- wipes the compiled style main (no styles: key) <- CSS survives So this PR introduced it. It fires on the external-resource branch with no `.ts` change at all. The evidence needed was already cached. `componentMetadataCache` holds the transform-time source with the `template:` / `styles:` VALUES blanked, and blanking only ever empties a delimited range — so an expression the strip cannot open survives verbatim, and the two stripped forms disagree exactly when the two sources disagree outside those fields. Matching strips is proof that the styles read here are the styles the component compiled with. This gates the destructive answer ONLY. Content that WAS read is still served on a mismatch, which is no worse than main, since main scanned the same disk source. `merged.length > 0` short-circuits, so the strip runs only when `[]` is on the table. Not done here, deliberately: the reviewer suggested caching metadata from each successful transform. `transform` does not re-run before the endpoint serves an inline `.ts` edit, so that cache is stale on the most common path. The four existing array-constant tests keep clearing, because disk and transform source are identical there. The gate separates "the compiler also saw this constant and resolved nothing", where clearing is exact, from "the compiler saw something else", where it is a guess. A new test pins the other side: identical sources, external-resource path, still clears. 347 unit tests pass, e2e stays at 37. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01665HouXD8imyMphWe1k7Dx --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 participant
Closes #457.
A component that lost its last style kept the old CSS applied until a full reload.
Cause
The generated HMR module opens with
...ClassName.ɵcmp, so every key it does not emit keeps its previous value.styleswas omitted for every "no styles" answer, so the stale array survived.The issue named one collapse point. There were four.
Fixing only the generator changes nothing, because
lib.rs:555never hands it aSome([])and the plugin never sends one.Measured before the fix:
null,undefined,[],[""],[" "]and the omitted argument all produced byte-identical output with nostyles:key.The fix
stylesis now three-valued, end to end:styles: [ … ][]styles: []— clearsnullnullwas overloaded, and that is the part worth reviewingIn the plugin,
nullmeant three different things:Case (b) is why the property cannot simply always be emitted — clearing on an unreadable decorator would wipe live CSS. So the distinction that matters is whether the answer is KNOWN, not whether it is empty.
I decided case (c) rather than leaving it open: a read failure is not evidence of stylelessness.
readStylespreviously swallowed failures in acatch {}and:730coalesced the result to[], which made a missing stylesheet indistinguishable from an empty one. Without the distinction, a truncate window during an editor's atomic write would report the component styleless and wipe its CSS.The file-level fallback branch keeps its old semantics and can never clear.
Why
styles: []actually worksTraced through the pinned Angular v22.0.0 submodule:
Styles are ref-counted per CSS string. Today the removed and re-added strings are the same, so the count dips and returns and the
<style>element never leaves. Withstyles: []nothing is re-added, the count reaches zero andelement.remove()runs.definition.ts:365readscomponentDefinition.styles || EMPTY_ARRAY, and an empty array is truthy, so an emitted[]survives intact.Two caveats found while reading that path, neither blocking: removal is skipped while
allLeavingAnimations.size !== 0(dom_renderer.ts:614), and it is gated onremoveStylesOnCompDestroy, default true.Verification
Binding contract after the fix:
cargo test --lib hmr::cargo test -p oxc_angular_compilerpnpm testpnpm test:e2eThe five new vitest cases cover both directions — the three clearing transitions, and the two that must NOT clear (an unreadable stylesheet, an unreadable style field).
One existing assertion changed deliberately
test/hmr-hot-update.test.ts, inserves no styles to a class that declares none, even beside a styled sibling, assertedexpect(body).not.toContain('styles:'). That encoded the old behavior. Its real intent — this class is not served its sibling's stylesheet — is still asserted by the marker check; the assertion now pins the explicit empty array instead. Two neighbouring tests passed either way and were tightened the same way.The e2e spec is the one that proves the user-visible fix
e2e/tests/hmr-style-removal.spec.tsloads a component with an inline style, confirms the dashed border and block host, emptiesstyles: [], waits for HMR, and asserts the border reverts tononeand the host toinline— with a DOM sentinel proving the browser did not reload and the websocket wire proving the server never asked for one.Red-checked: with the generator guard restored and the binary rebuilt, it fails with
Expected: "none" / Received: "dashed".Note for the reviewer
Two transitions the issue lists still take a full reload rather than the hot path:
styleUrls: ['x'] -> [], and removing thestyles:key outright.stripComponentMetadatablankstemplate:andstyles:but notstyleUrls:, so those edits break the byte-equality check and force a reload. That masking is pre-existing, is not a wrong result, and is left alone here.Note
Medium Risk
Changes dev-only HMR style resolution; wrong unknown-vs-empty handling could clear live CSS during failed reads, though the new
completeflag and tests target that edge case.Overview
Fixes #457: after HMR, a component that went from having styles to none kept old CSS until a full reload, because update modules spread
...ClassName.ɵcmpand omittedstylesfor every “no styles” case.End-to-end contract:
stylesis now three-valued — non-empty arrays update CSS,[]means “definitely no styles” and must emitstyles: []to clear the runtime, andnullmeans unknown so the key stays omitted and existing styles are preserved.The HMR module generator emits
styles: []when givenSome([])instead of skipping the property.compile_for_hmr_syncno longer folds definitive empty caller input intoNoneafter encapsulation. The Vite plugin tracks stylesheet read completeness (failed/missing reads vs successful empty files) so atomic-write gaps are not treated as “clear styles,” while still allowing partial updates when one of severalstyleUrlsfails.API docs (
index.d.ts, Rust doc comments) describe the semantics. Tests add a Rust unit case, a Playwright e2e for visible CSS removal without reload, and many Vitest scenarios for clear vs must-not-clear paths.Reviewed by Cursor Bugbot for commit 8ae92a1. Bugbot is set up for automated code reviews on this repo. Configure here.