Find a scoped <style> regardless of what precedes it - #424
NullVoxPopuli merged 4 commits into
Conversation
The Template visitor picked the block to extract with `node.body.find((n) => n.tag === 'style')` -- the first style element, scoped or not -- while the ElementNode visitor removes every scoped element. So a global <style> sitting earlier in the template made the guard below it false, skipped the whole extraction branch, and left the scoped tag to be deleted anyway. Its CSS was emitted nowhere and nothing said so; addInfo() never ran either, so the classes were never collected and the CSS could not have matched its elements in the first place. Selecting by the scoped attribute rather than the tag fixes that ordering. A second <style scoped> was lost the same way, since only the first is ever extracted and the rest are removed regardless. Supporting several blocks is a larger question, so this refuses one out loud instead of answering it quietly: emitting the first and discarding the second is the one behaviour nobody wants. Fixes auditboard#423 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The count check reads better before anything is pulled out of the list, and the hasScopedAttribute guard around the body was redundant -- the filter above already guarantees it. An explicit early return now covers the only case that guard was really catching: no scoped <style> in the template at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| * rule; the ordering is the only thing under test. | ||
| */ | ||
| describe('a scoped <style> is found regardless of what precedes it', () => { | ||
| it('extracts the scoped CSS when a global <style> comes first', async () => { |
There was a problem hiding this comment.
this test would be better as an inline snapshot. the individual expects lose context / "the plot", as claude often does
There was a problem hiding this comment.
(I think all the new tests, actually)
There was a problem hiding this comment.
Both new tests now assert with snapshots, in 8449956.
The template snapshot carries the whole story on its own -- scoped class postfixed, global block shipped as written, scoped tag removed:
[
"<div class="scoped_e65d154a1">hi</div>
<style>
.global { color: red; }
</style>",
]
plus the emitted virtual import, same shape as the lang="scss" tests above.
That made the third test ("leaves a preceding global <style>") a strict subset of this one, so I dropped it rather than keep a near-duplicate. Say the word if you would rather keep it separate.
The second-block case stayed a toThrow, matching babel-plugin.test.ts:99. Babel prefixes the message with the absolute filename, so toThrowErrorMatchingInlineSnapshot writes my home directory into the snapshot and only passes on this machine.
Both still fail against the pre-fix plugin -- I checked by swapping in the version from main.
There was a problem hiding this comment.
Went back over the whole review. The four comment deletions were one note, not four, and I had only applied it to the tests -- template-plugin.js still carried the same kind of prose I had added above the filter and the length check. Both gone in cee6d1a, leaving the one-line comment that was there before this branch. The error message covers what the second one was saying.
The diff against main for that file is now just the behaviour change.
One snapshot of the whole template says more than a pile of expects: the scoped class is postfixed, the global block ships as written, the scoped tag is gone. That subsumes the separate "leaves a preceding global <style>" test, so it goes. The second-block case stays a toThrow, matching babel-plugin.test.ts. Babel prefixes the message with an absolute filename, so a snapshot of it would only pass on the machine that wrote it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Same note the tests got: the code and the error message say this already. Leaves the root-level comment that was here before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
auditboard#424 fixed both halves: a global <style> before a scoped one now extracts, and two <style scoped> blocks now throw at build time rather than quietly dropping the second. Nothing is linted here that the build discards, and auditboard#423 is closed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fixes #423.
The bug
The
Templatevisitor picked the block to extract by tag alone:while the
ElementNodevisitor removes every scoped style element. Those two disagree, and the disagreement loses CSS: a global<style>earlier in the template makesfindreturn the wrong element,hasScopedAttributefalse, and the entire extraction branch is skipped — but the scoped tag is still deleted from the compiled template. The CSS is emitted nowhere, and no error or warning is produced.addInfo()never runs either, so the classes are never collected. Even if the stylesheet had been emitted, the elements keep their unpostfixed class names and nothing would have matched.Reversing the two blocks makes it work.
The fix
Select by the scoped attribute rather than the tag, which is the fix suggested in the issue. That closes the ordering case.
It does not close the second symptom the issue mentions in passing: a template with two
<style scoped>blocks loses the second one the same way, because only the first is ever extracted and the rest are removed regardless. I confirmed the one-line fix alone leaves that case broken.Supporting several blocks per template is a larger design question. Rather than answer it quietly, this refuses the second block with an explicit error, since emitting the first and silently discarding the second is the one behaviour nobody wants:
Happy to change that to "merge them and emit both" if that is preferred — it is a small change from here, and the tests say which behaviour is intended either way.
Tests
Two in
template-plugin.test.ts, using the file's existingtransform/virtualImportUrlsOf/templateContentsOfhelpers. Both fail against the plugin as it stands onmain: the first emits no import and leaves the class unpostfixed, the second resolves instead of throwing.The first snapshots the compiled template and the emitted virtual import. That also pins the other side of the fix, since the snapshot shows a genuinely global
<style>still shipping as written rather than being extracted or removed.The second stays a
toThrowrather than a snapshot: babel prefixes the message with an absolute filename, so a snapshot of it would only pass on the machine that wrote it.Full suite: 205 passed (was 203), no regressions.
Relationship to #422
#422 adds a stylelint
customSyntaxthat lints every root-level<style scoped>, which is how this was found: the linter reports problems in CSS the build discards. #422 documents that divergence as a known limitation and links here; it does not fix it.The two are independent and can land in either order. I verified this branch's tests and fix pass against #422's code as well as against
main— the fix touches only theTemplatevisitor body, while #422 touches the imports and moves the attribute helpers into a sharedstyle-tag.js, so they occupy different regions.Whichever lands second: #422's "Known limitations" bullet about the build dropping scoped CSS becomes false once this merges, and its link to #423 becomes a dangling reference to a closed issue. That bullet should be removed.