fix: handle @scope at-rule edge cases#91
fix: handle @scope at-rule edge cases#91merklebonsai wants to merge 3 commits intocss-modules:masterfrom
Conversation
|
Under honest disclosure - this PR is produced together with Claude. Yet, this is not a slop; more of an opposite actually. I've specifically ran Claude to identify edge cases I'm missing (e.g. comments that include |
| } | ||
|
|
||
| return { start, end }; | ||
| } |
There was a problem hiding this comment.
Is it AI generated code? because I see a lot of rooms to improve
There was a problem hiding this comment.
Ask Claude to simplify it and make faster, he knows how to do it
There was a problem hiding this comment.
Fair, thanks for comment - I've been aimed (and aimed Claude) at spec compliance firsthand, not primitives like postcss-value-parser reuse.
Optimized in 988304c
Tried to benchmark, and the difference is very subtle between all versions, starting with pre-commits. With 5000 iterations run, it's within few nanosecs per run (some paths are 25-26 usec, some paths are 100-102 usec). So there was almost no degradation in performance initially - and sadly, I was unable to improve it more. So, only did code quality improves.
If you have some specific code style preferences (e.g. use switch in parseScopeParams) I can do it, just need an advice on preferred shape here.
Replace the hand-rolled tokenizer with a postcss-value-parser pass (already a dependency). Match three explicit grammar shapes that mirror the spec: (start), to (end), and (start) to (end). The `to(...)` form without whitespace, which value-parser tokenizes as a single function, is normalized into [to, (...)] upfront so all spacings collapse to one match. Also dedupe the at-rule body iteration that was identical between the @scope branch and the non-scope branch. Net: -93 LOC vs the prior @scope chunk, same end-to-end performance.
|
We still support old nodejs, yeah I know we should drop them, but let's avoid breaking changes here |
2 participants
Two related fixes for the
@scopeat-rule handler. Two atomic commits.Commit 1 —
fix: parse @scope params with paren-aware tokenizerFixes #90.
The current
@scopehandler splitsatRule.paramson the substring"to":This naively matches
toanywhere in the params, including inside class names (.buttoncontainsbu+to+n; same goes for.tooltip,.toolbar,.photo,.story,.into-view, etc.) and inside attribute selector values ([data-section="footer"],[role="button"]). The result is malformed CSS with extrato ()clauses and class names truncated mid-token, that never match the DOM.The fix replaces
.split("to")withparseScopeParams— a small character-by-character tokenizer that walks the params with paren-depth and string-literal tracking. Thetokeyword is detected only at depth 0, between two parenthesized groups, with a word boundary check so identifiers starting withto(liketools,topology) don't match.The same tokenizer also handles two adjacent legal-CSS edge cases that the old
split("to")would have mishandled regardless of the substring issue:/* something with parens */)\(,\)) — legal per the CSS spec, occasionally emitted by CSS-in-JS pipelinesWhen params can't be parsed, the plugin now emits
result.warn(...)via the postcss API so module-name leakage is diagnosable rather than silent.Commit 2 —
fix: prevent crash on body-less @scope at-ruleThe
@scopebranch in the at-rule walker callsatRule.nodes.forEach(...)unconditionally. When postcss parses an@scopeat-rule with no body (e.g.@scope (.foo);),atRule.nodesisundefinedand.forEachthrowsCannot read properties of undefined (reading 'forEach').The non-scope at-rule branch already handles this with
else if (atRule.nodes). This commit adds the same guard to the@scopebranch.Why we hit this
We're shipping a visual app builder that emits CSS with
@scoperules where component-boundary markers can land in any of the params positions, and component names are user-supplied. Most names hit at least one of the failure modes (any class containing the substringto). Without these fixes,next build— which uses postcss-modules through webpack by default — silently produces broken styles in our generated output. Our codegen needs arbitrary user input to round-trip correctly through every CSS Modules mangler in the ecosystem.Test plan
tosubstring (canonical Bug in handling @scope rule - "to" as a part of a selector #90 case)toin scope-endtotoinside but no scope-end clauseTOkeyword (CSS keywords are case-insensitive):is()/:not()selectors (parser depth correctness)toand unbalanced parens\(in identifier@scopeno longer crashesyarn lint(eslint + prettier) clean.