[babel-plugin] only substitute referenced constants in processStylexRules#1700
Open
henryqdineen wants to merge 1 commit into
Open
[babel-plugin] only substitute referenced constants in processStylexRules#1700henryqdineen wants to merge 1 commit into
henryqdineen wants to merge 1 commit into
Conversation
|
@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. |
…ules processStylexRules looped over every constant in constsMap and replaced it in every non-constant rule -- O(rules x constsMap). On a real ~15.7k-rule, ~3.3k-const build that was ~7.5s, almost entirely this loop. Substituting only the references actually present in each rule drops it to ~28ms (~27% of rules reference a constant, ~1.1 distinct refs each). It also makes override resolution deterministic. A defineConsts value can be a var(...); used as a property key it overrides that variable in scope (facebook#1219), and processStylexRules rewrites the declared key to the variable the const aliases. The previous loop applied that rewrite cumulatively across all of constsMap, so for chained aliases it followed the chain a variable distance depending on order. Now: - value substitution runs once per referenced var (values are pre-resolved by resolveConstant; only var(--x, fallback), which is overridable, is left); and - the override-key rewrite is a single pass that resolves each declaration one independent step, so it cannot cascade across declarations. The per-rule scan matches any custom-property key, including non-ASCII (e.g. --café), so those substitute correctly. resolveConstant is left untouched, so it still only pre-collapses ASCII alias chains -- a non-ASCII const->const chain resolves a single step here rather than through to its terminal. That's a pre-existing limitation we noticed but did not change (terminal non-ASCII keys resolve fine; escaped-delimiter names like --foo\:bar are also out of scope). Tests: bare vs fallback chain resolution; single- and multi-declaration override rewrites resolve one step and are order-independent; non-ASCII key (terminal, plus the unresolved-chain limitation); no-reference / no-constant paths; one end-to-end cross-file case (a @media breakpoint const consumed from a separate module via a real import) through transform(). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
3e83317 to
1326301
Compare
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 ?
This PR makes
processStylexRulessubstitute only the constants each rule actually references, instead of replacing every constant inconstsMapinto every rule.That loop was
O(rules × constsMap)and dominated the function. On a real ~15.7k-rule, ~3.3k-const HubSpot production app,processStylexRulestook ~9.2s — almost all of it in this loop. This PR brings it to ~29ms (~317×); output was byte-identical on our app. The only behavior changes are the narrow edge cases detailed below.A single targeted pass is correct because the existing
resolveConstantstep already collapses constant alias chains up front, so each value is already fully resolved — substituting every referenced var once is enough, with no need to re-scan to follow chains.It also fixes a latent nondeterminism: the old override-key rewrite (#1219) was order-dependent for multi-hop const→const alias chains. That's an edge case — the common pattern (a const aliasing one externally-defined variable) is identical before and after. Details in Additional Context.
Linked PR/Issues
Relates to #1219 (
var()overrides — using a const as a property key to override the var it aliases) and #1460 (---prefixed verbatim const keys, which let one const alias another by name). Together they're the source of the const aliasing / override behavior this change has to preserve. No separate tracking issue.Additional Context
This is an edge-case behavior change. The common pattern — a const aliasing a single externally-defined variable (and how we use it) — is byte-identical before and after. Output only changes for multi-hop const→const chains (one const naming another — which needs the verbatim
--keys from #1460) or rules with multiple interacting override declarations, and only with fallback-form refs (var(--x, fallback)); bare chains are pre-collapsed byresolveConstant, so they're identical on both branches. The full delta is exactly three tests (see Tests).The mechanism: a
defineConstsvalue can be avar(...); used as a property key it emits a declaration overriding that variable (#1219), andprocessStylexRulesrewrites the declared key to the variable the const aliases. The old loop applied that rewrite cumulatively across all ofconstsMap, so for a chain it followed a variable distance depending on order:Overriding
--primaryemitted--brandPrimary:one way and--blue500:the other — same source, different CSS. Now the override-key rewrite is a single pass resolving each declaration one independent step, so it targets the variable the value actually reads and can't cascade or depend on order.Key grammar. The per-rule scan matches any custom-property key, including non-ASCII (e.g.
--café), so terminal non-ASCII keys substitute correctly.resolveConstantis unchanged (byte-identical to main).Benchmark. We captured the exact
Rule[]one production HubSpot app handsprocessStylexRules(15,690 rules — 3,281 constant + 12,409 CSS) and timedmainvs this PR on it (median of 7, byte-identical output, 326,173 bytes): 9,152ms → 28.8ms (~317×). The change tradesO(rules × constsMap.size)(areplaceAllper constant per rule) forO(var/declaration tokens)(a scan + an O(1) lookup pervar(--…)/--…:token), so cost no longer grows with constant count.Cost model, sweeps, and the one slower case
New cost tracks how many
var()references the rules contain, not how many constants exist. Atomic rules are a single declaration each, so they reference only a handful of vars (our dump averaged ~0.4 per rule). Each row below varies one axis and feeds both builds identical input (output byte-identical in every case); rules = total rules, consts = number of constants, refs/rule =var()references per rule:Old time grows with the number of constants; new time stays flat and only grows with how many
var()refs the rules actually contain.The new code is only slower when there are very few constants and a rule is packed with many non-constant
var()refs (e.g. 3 constants and 30 refs in one rule → ~1.7×, 41→70ms). It takes both at once, atomic CSS produces neither (one declaration per rule), and even then it's only tens of ms.We considered closing even that case: instead of scanning for every
var(--…)and checking each against the map, build one regex from the actual constant names —var\((--brandPrimary|--accent|…)\)— so the scan only matches real constants. Measured, it's about even on our dump and ~10× faster in the packed case (and stays fast even with thousands of constants). We kept the simplerconstsMap.get(match)here for readability.Tests: all existing
processStylexRulestests pass unchanged. Added coverage: value substitution, single-alias overrides, bare/fallback chain resolution, non-ASCII keys (+ the unresolved-chain limitation), no-reference / no-constant paths, and an end-to-end cross-file case (adefineConsts@mediabreakpoint consumed via a realimport) throughtransform(), snapshotting metadata + CSS.Exactly three of the added tests fail on
main— the complete behavior delta:mainover-resolves it)Questions for maintainers
processStylexRules(new behavior opt-in)?---prefixed const key directly — one const aliasing another by name, used as an override key, via a fallback-form chain. If verbatim const keys are meant to be opaque (an implementation detail, not a stable name to alias/override by), these scenarios are arguably out of contract and the behavior change is moot. Is relying on verbatim const-key names intended, or something to discourage? It would let us drop the most esoteric tests here.Pre-flight checklist
🤖 Generated with Claude Code