| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
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
…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
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_886d9e28-3c9e-4690-913d-9d8e7651cc5b) |
Sorry, something went wrong.
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
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_e840fb58-37aa-4c8b-a8e0-1461aebdd277) |
Sorry, something went wrong.
|
Confirmed and fixed in 873d190. You were right, and the trigger is wider than the finding says. Reproduced, and it is this PR's regressionReal dev server, disk holding styles: STYLE_ARRAY, an upstream load expanding it to a literal, editing only the external .html: this branch (1f98853) template: function S1Component_Template(...){...},
styles: [], <- wipes the compiled .SMARKER
main (ab79282) template: function S1Component_Template(...){...},
}); <- no styles: key, CSS survives
It fires on the external-resource branch with no .ts change at all, and for any form the extractor cannot fold — array constant, imported constant, .concat(...) — all three cleared here and none cleared on main. It does not need a third-party plugin. This plugin's own fileReplacements diverges by itself: index.ts:858, const actualId = pluginOptions.fileReplacements?.[id] ?? id, where code is the content of id and the endpoint later reads actualId from disk. Verified on that path specifically — the dispatched id carried the replacement file, and the fix refuses the clear there. One correction to the finding's phrasing: enforce: 'pre' alone is not the trigger. This plugin declares transform: { order: 'pre' }, and Vite sorts by hook order before array position, so a plain enforce: 'pre' transform function loses to it. What actually wins is any load hook, or any transform: { order: 'pre' }. The fix, and why not the one you recommendedI did not cache metadata from each successful transform. transform does not re-run before the endpoint serves an inline .ts edit — measured, transformCalls stays at 1 — so that cache would be stale on the most common path. The evidence 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: stripped(TRANSFORM-TIME) "... styles: [],\n})..." stripped(DISK) "... styles: STYLE_ARRAY,\n})..." The two disagree exactly when the two sources disagree outside those fields. That is the proof needed, and it was simply never consulted on the path where the clearing fires. const styles: string[] | null =
merged.length > 0 || (external.complete && compiledFromThisSource()) ? merged : nullThis gates the destructive answer ONLY. Content that WAS read is still served on a mismatch — no worse than main, which scanned the same disk source. merged.length > 0 short-circuits, so the strip runs only when [] is on the table. Key alignment checked two ways, since fileReplacements is exactly where it could break: componentsByFile.set and componentMetadataCache.set are three lines apart under the same actualId, and the endpoint's guard already refuses any request whose resolvedId is not a live key of that map — so a request reaching the styles code has proven the keys match. VerificationThree tests red-checked, failing with expected '// HMR update for: …' not to contain 'styles:' and styles: [], in the body. One counterweight test proves the gate is not too wide: identical disk and transform source, class already at styles: [], edited through the external-resource path — still clears. It passed before and after, which is what makes it a real guard. 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. 343 -> 347 unit tests; e2e stays at 37. Two neighbouring hazards, left aloneBoth measured in the same scenario, both identical on main, so neither belongs to this PR:
Worth their own issue rather than a fix folded into this one. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Closes #456. Two commits: the functional swap, then the deletion it enables.
The problem
The @ng/component HMR 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 text 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.
Five review rounds on #455 each patched one syntactic form and revealed the next. This removes the text locators from the resolution path instead of teaching them to fold constants.
Two things the issue got wrong, both found by measurement
1. The metadata was not reachable. The issue assumed extract_component_metadata_sync was available. It is fully written and correct but carries no #[napi], so it was not on the JS surface at all. Adding the attribute is the entire Rust change.
2. There is nothing to cache. The issue suggested the data might already be in hand at transform time. It is not: result.styleUpdates is always empty and templateUpdates' values are discarded, only the keys kept. And a probe showed transform does NOT re-run before the endpoint serves an inline edit — transformCalls stays at 1 — so a transform-time cache would have served stale resources.
That turned out not to matter. The endpoint already does this per request:
So this is a one-for-one swap of one call for another. No cache, no staleness, no ordering hazard. ~0.016 ms of Rust against the ~0.30 ms async hop it paid before.
Why the per-class answer is definitive
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 (transform.rs:4061), 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. All matched — including the one that looks like a partial read:
Two of three in both. The dropped import genuinely is not a stylesheet this component gets, so serving two is exact, not partial. The same held for spreads, let bindings, concatenation, and cross-file imports.
A class MISSING from the metadata is equally definitive. decorator.rs:90's ? drops a whole class when one key will not resolve — and measured, the real compile skips it too: decorator left intact, no ɵfac, no ɵcmp, its stylesheet not even in dependencies. Same for import { Component as Cmp }, a class nested below top level, and a @Directive.
So both fallbacks are deleted rather than narrowed.
What this retires, and what it does not
The unreadable state existed only because the text scan disagreed with Rust. With Rust as the source, that disagreement cannot occur.
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 — a filesystem question, still real, and still the thing that stops a truncate window during an atomic write from wiping live CSS. That code is byte-identical.
Three assertions flipped, deliberately
Three tests used styles: SOME_ARRAY_CONST and asserted no styles: key, on the stated theory that "the Rust extractor folded the CSS out of the constant". It does not. OXC's collect_string_consts folds STRING-valued consts; extract_string_value never walks an array. Verified against compiler output rather than reasoned:
const ARR = ['.FROM_ARRAY_CONST{...}'] @Component({ styles: ARR }) -> ɵcmp has NO styles property at all @Component({ styles: ['.LIT{}'] }) -> styles:[".LIT[_ngcontent-%COMP%]{}"]That component compiles with no styles, so styles: [] clears nothing that ever existed. All the #457/#461 read-failure guards are untouched and green.
Commit 2: the deletion was far smaller than expected
I predicted ~139 tests would delete and roughly half the scanner would go. Wrong, in the safe direction — nine of ten candidate helpers turned out to be live, all reachable from locateStylesInArgs / locateTemplateInArgs through findFieldInArgs. Only hasInterpolation was genuinely dead.
Of 206 tests: 22 unchanged, 78 keep their title with the call swapped to a surviving locator, 26 renamed because the old title named a concept that is gone, 76 deleted. Comment handling, decoy tokens and Unicode identifiers were all salvaged — the strip path still parses decorator text and still has to get them right.
Also deleted: inlineTemplateCache and inlineStylesCache, written and pruned and refreshed on hot update, and read by nothing — true before this PR.
stripComponentMetadata and its closure are untouched. It decides full reload versus hot update.
Verification
The six new tests are the const-folded cases the text scan could never read, each with a styled sibling in the same file to prove the contamination is gone. All six fail on ab79282; red-checked by restoring the old plugin file. One of them pins the invariant the whole design rests on: it runs the plugin's own transform, slices the emitted ɵcmp, and asserts the endpoint serves exactly what the compiler put there. If extractor and compiler ever diverge, that test moves.
Commit 2's behaviour is proven beyond e2e: 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.
Notes for the reviewer
Note
Medium Risk
Changes dev-server HMR resolution and style-update semantics; behavior is heavily tested but wrong per-class updates would show up only in multi-component or non-literal decorator setups.
Overview
Fixes per-class HMR (#456) by having the @ng/component endpoint load each class’s templateUrl / template / styleUrls / styles from extractComponentMetadataSync (same Rust path as compile) instead of scanning decorator text and falling back to the file-level URL union—which could serve a sibling’s template or CSS in multi-component files.
The Rust fn is now #[napi]-exported; the plugin drops inlineTemplateCache / inlineStylesCache, file-level fallbacks, and the old text-based extractors. decorator-fields is trimmed to what stripComponentMetadata still needs for HMR vs full-reload; URL/style literal parsing helpers are removed.
Style clearing only treats an empty merged list as definitive when filesystem reads are complete and disk source matches the transform-time stripped cache—so disk/transform mismatches (array consts, imports, etc.) omit styles instead of wiping live CSS.
Tests and e2e comments are updated; large decorator-fields and hmr-hot-update coverage adds const-folded / sibling-contamination cases.
Reviewed by Cursor Bugbot for commit 873d190. Bugbot is set up for automated code reviews on this repo. Configure here.