Repository navigation
fix(css): read a block owner's at-keyword through _atKeywordOf when ordering declarations - #22586
Conversation
… ordering declarations #22569 removed `path.atKeyword` while #22513 added a call to it, so minifying any at-rule block holding more than one declaration, such as `@font-face`, threw. Claude-Session: https://claude.ai/code/session_01ViD41i2KdwwGxrDk7yrjeD
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe parser now checks nested group at-rules with ChangesCSS at-rule parsing
Suggested labels: Suggested reviewers: Priority: ⬇️ Low Change: Bug fix Merge Risk: ⚪ Minimal · up to The affected 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
lib/css/syntax-parser.jsESLint failed to execute (timeout). test/unitCases/CssSyntax.unittest.jsESLint skipped: the matched ESLint configuration already failed (timeout). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
… ordering declarations Ported from #22586: main throws minifying an at-rule block of several declarations, which fails this branch's CSS invariants and code-size jobs. Claude-Session: https://claude.ai/code/session_01ViD41i2KdwwGxrDk7yrjeD
|
This PR is packaged and the instant preview is available (135a87b). Install it locally:
npm i -D webpack@https://pkg.pr.new/webpack@135a87b
yarn add -D webpack@https://pkg.pr.new/webpack@135a87b
pnpm add -D webpack@https://pkg.pr.new/webpack@135a87b |
Types CoverageCoverage after merging fix/css-declaration-order-at-keyword into main will be
Coverage Report |
Generated code sizeComparing base ( Warning The baseline is
Read 34 asset(s) changed size, test untouched, biggest 20 by raw or gzip change
1 asset(s) changed size, test edited
No runtime gained or lost a runtime module. Built |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #22586 +/- ##
==========================================
+ Coverage 95.54% 96.19% +0.64%
==========================================
Files 774 774
Lines 128230 128374 +144
Branches 42484 42525 +41
==========================================
+ Hits 122514 123485 +971
+ Misses 5716 4889 -827
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…n `node:` is supported (#22593) * fix(externals): keep a core module external as the code spells it when `node:` is supported #22565 also prefixed a bare core module external with `node:` whenever `output.environment.nodePrefixForCoreModules` was on. An unversioned `node` target turns that option on, so bundles run on Node 12 required `node:worker_threads` and failed. A bare external now stays bare again; only a universal bundle gains the prefix. Claude-Session: https://claude.ai/code/session_01ViD41i2KdwwGxrDk7yrjeD * fix(css): read a block owner's at-keyword through `_atKeywordOf` when ordering declarations Ported from #22586: main throws minifying an at-rule block of several declarations, which fails this branch's lint, unit and CSS checks. Claude-Session: https://claude.ai/code/session_01ViD41i2KdwwGxrDk7yrjeD
Summary
mainthrows when minifying an at-rule block with more than one declaration. #22569 removedpath.atKeywordin favor of_atKeywordOf, and #22513, which merged after it, added a newpath.atKeyword(owner)call. Each PR was green on its own. Together they break the CSSinvariantsjob and thecode-sizejob on every open PR.TypeError: path.atKeyword is not a function@font-face{src:url(a.woff);font-family:x}yarn test:invariants:cssreports 2 findings onmain(both this TypeError) and 0 with this fix.What kind of change does this PR introduce?
fix
Did you add tests for your changes?
Yes, an
@font-facecase intest/unitCases/CssSyntax.unittest.js. It fails onmainwith the TypeError and passes with the fix.Does this PR introduce a breaking change?
No.
If relevant, what needs to be documented once your changes are merged or what have you already documented?
n/a
Use of AI
Written with Claude Code. It traced a CI failure on another PR back to
main, found the two conflicting commits, and wrote the fix and test. I reviewed the change.🤖 Generated with Claude Code
https://claude.ai/code/session_01ViD41i2KdwwGxrDk7yrjeD
Generated by Claude Code
Summary by CodeRabbit
@font-facerules during minification.