[babel-plugin] Deterministic ordering for px min/max-width defineConsts breakpoints - #1652
Merged
mellyeliu merged 7 commits intoSep 4, 2026
Conversation
…ts breakpoints When breakpoints are defined with defineConsts, the Babel transform only sees var(--hash) placeholders at create() call time, so enableMediaQueryOrder cannot apply and ordering falls back to alphabetic — which has nothing to do with breakpoint pixel values. This fixes ordering at CSS generation time (processStylexRules), where constants are already resolved. Constants are now resolved before sorting rather than after, and min-width queries are sorted ascending and max-width queries descending by px value. Only pure min-width or pure max-width queries are sorted; range queries with both dimensions fall through to preserve comparator transitivity. Uses MediaQuery.parser (from style-value-parser) for extraction so that negated queries, screen-and queries, and CSS Level 4 range syntax are all handled correctly. MediaQuery and MediaQueryRule are exported from style-value-parser's public API to support this. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
henryqdineen
requested review from
mellyeliu,
nmn and
vincentriemer
as code owners
May 8, 2026 21:01
|
@henryqdineen is attempting to deploy a commit to the Meta Open Source Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
👋 Anything we can do to help move this PR forward? We're definitely stubbing our toes on this while trying to setup responsive styles with StyleX! |
Refresh the legacyDisableLayers inline snapshot to match the class and custom property names produced after facebook#1834 removed property-prefixed debug class names. Ordering intent is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Hoisting constant substitution above the sort means getLogicalFloatVars inspects resolved values. Pin that: a float arriving via a defineConsts constant now emits the :root block defining --stylex-logical-start, where previously the declaration referenced an undefined custom property and computed to none. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two bugs in the px min/max-width breakpoint sort: A negated width bound inside a conjunction was silently discarded, so "(min-width: 500px) and (not (max-width: 700px))" -- effectively "width > 700px" -- sorted by its 500px bound and lost the cascade to "(min-width: 600px)" above 700px. The walk now bails on any `not`, `or`, negated media type, or non-px bound, and requires exactly one bound. The sort also assumed "@media" started the rule, so a breakpoint nested in another at-rule never sorted. Keys are now built from the whole at-rule chain, and only rules sharing the same surrounding conditions compare against each other. Parsing moves out of the comparator into a chain-keyed cache; it ran O(n log n) times before, which made large sheets pathologically slow. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The breakpoint order only applied to pairs of rules bounded on the same side, so some pairs were decided by width and others by declaration text. That is not a consistent comparator, and it produced a cycle: a min-width 500px z-index rule, a max-width 300px margin rule, and a min-width 900px align-items rule ordered a < c < b < a, so the output depended on input order. Six permutations gave three different results. Comparison is now lexicographic over property name, then width, then declaration text, with every width pair getting an answer: bounded rules ahead of unbounded, grouped by at-rule context, then min-width ascending before max-width descending -- the mobile-first order used by sort-css-media-queries. Grouping by property name rather than the whole declaration is what makes this work. Two breakpoints for one property differ in value by definition, so comparing declaration text first would preempt the breakpoint order entirely. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mellyeliu
reviewed
Sep 4, 2026
| v != null && | ||
| typeof v === 'object' && | ||
| typeof v.value === 'number' && | ||
| v.unit === 'px' |
Member
There was a problem hiding this comment.
Could we support unitless zero and case-insensitive px values here?
Collaborator
Author
There was a problem hiding this comment.
Thanks @mellyeliu. 5001691 should take care of that. Sorry this PR is not super fresh in my memory so I have been trying to refamiliarize myself.
Media feature names and units are both ASCII case-insensitive, so
"(MIN-WIDTH: 700Px)" and "(min-width: 900PX)" were parsed but never
sorted. Zero is also the one length valid without a unit, and the parser
reports it as a bare number rather than a length, so "(min-width: 0)"
was skipped too. Zero now counts as a bound in any unit, or none.
Unitless non-zero values are still skipped: "(min-width: 700)" is
invalid CSS and the browser drops the query, so treating it as 700px
would order the sheet by a rule that never matches.
A ratio on a width feature ("(min-width: 16/9)") is array-shaped rather
than a length, and now bails explicitly instead of relying on a missing
property read -- and, as with a negated bound, cannot let a sibling px
bound stand in for it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mellyeliu
approved these changes
Sep 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed / motivation?
Adds deterministic CSS ordering for
min-widthandmax-widthmedia query breakpoints defined withdefineConsts. Previously, these rules were ordered alphabetically at CSS generation time, which could put a wider breakpoint before a narrower one and cause the wrong styles to win the cascade.Given this setup:
Before (broken) — alphabetic hash ordering could put desktop first:
After (fixed):
Why the existing transform doesn't help
enableMediaQueryOrderrewrites overlapping media queries into non-overlapping ranges, but it runs during the Babel transform wheredefineConstsvalues are only visible asvar(--hash)placeholders — not as literal@mediastrings. The rewrite can't apply, so ordering was left to the alphabetic fallback.The right place to fix this is CSS generation (
processStylexRules), where constants are already resolved. This PR resolves allvar(--hash)references before sorting rather than after, then sortsmin-widthqueries ascending andmax-widthqueries descending by px value.Hoisting substitution also means
getLogicalFloatVarsnow inspects resolved values (hencenonConstantRules→sortedRules), which incidentally fixes a latent bug: afloatwhose value arrived via adefineConstsconstant previously emittedfloat: var(--stylex-logical-start)without ever emitting the:rootblock defining that variable, so the declaration referenced an undefined custom property and computed tonone.MediaQuery.parser(already instyle-value-parser) is used for value extraction. This means CSS Level 4 range syntax like(width >= 768px)is normalised tomin-widthand sorted correctly for free. Queries without a single-sided px bound are skipped: anynotoror(which can widen the matched range or split it in two), a negated media type, a non-px bound, or more than one bound. Walking the parsed structure matters here rather than string-matching —(min-width: 500px) and (not (max-width: 700px))is effectivelywidth > 700px, so sorting it by its literal500pxputs it ahead of(min-width: 600px)and loses the cascade above 700px. Comma-separated OR queries like@media (min-width: 600px), (min-width: 900px)fall through to the existing sort unchanged — there is no single value that correctly represents their sort position.MediaQueryandMediaQueryRuleare now exported fromstyle-value-parser's public API to support this.The sort is a total order, so output can't vary with input order. Comparison runs over property name, then width, then declaration text, and every width pair gets an answer: bounded rules ahead of unbounded, grouped by surrounding at-rule context, then
min-widthascending beforemax-widthdescending — the grouped mobile-first ordersort-css-media-queriesuses. Grouping by property name rather than the whole declaration is load-bearing: two breakpoints for one property differ in value by definition, so comparing declaration text first would preempt the breakpoint order entirely.Breakpoints nested inside another at-rule sort as well. The key is built from the whole at-rule chain, and rules only compare against others sharing the same surrounding conditions — so
@supports (display: grid) { @media (min-width: …) }orders correctly without cross-sorting against a different@supports.Parsing happens once per distinct at-rule chain, not inside the comparator — which runs O(n log n) times.
Why only
min-widthandmax-width?These are sortable because they describe conditions along a single linear dimension — any two values are comparable, their overlaps are always in the same direction, and the cascade intent ("larger breakpoint overrides smaller") maps directly to CSS order. Other media features don't have these properties:
rem/emrequire a root font size assumption unavailable at build time, and features likeorientationorprefers-color-schemeare independent conditions with no meaningful ordering between them. For comparison,sort-css-media-querieshard-codesem/rem→ ×16; falling through seems better than guessing a root font size.Applying the full
enableMediaQueryOrder-style rewrite at CSS generation time would require knowing which rules are siblings in the same property's condition group. That grouping information is lost by the time we reachprocessStylexRulesand preserving it would be a more invasive change.Linked PR/Issues
Fixes #1646.
Additional Context
Prior art.
sort-css-media-queries(viapostcss-sort-media-queries) uses the same grouped mobile-first order, but extracts values by regex on the raw query string, taking the first length match — sonotis detected with a substring test and comma-separated lists aren't parsed at all (yunusga#37). Its comparator is also non-transitive, producing different output across browsers and input orders (OlehDutchenko#12). Using the parser plus a total order avoids both classes of bug.The
defineVarsresponsive value ordering issue noted in the issue comments (usingremrange-syntax queries) is not addressed here and likely warrants a separate fix.Pre-flight checklist
Contribution Guidelines