Repository navigation
test: the test runtime keeps every StyleX rule its dev runtime keys by an at-rule prelude - #8695
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This looks ready to merge: the new runtime injection restores every rule StyleX 0.19.0's dev runtime drops under a conditional prelude, and the new guard test fails without it and passes with it. Two optional follow-ups:
stylexInject.tsrecords which preludes are taken but not the rule that took each one, so the same declaration compiled twice would leave that first rule in both sheets; addinginserted.add(text);besidepreludes.add(prelude);closes it.- Later
@starting-stylerules are dropped by the same keying and are not restored, so the description's "holds every rule StyleX generated" is true for conditional preludes only; extend the wrapper or narrow that sentence.
[Automated review]
| const prelude = text.slice(0, text.indexOf('{')).trim().replace(COMMA, '$1'); | ||
| if (!preludes.has(prelude)) { | ||
| // The first rule under this prelude: StyleX inserted it. | ||
| preludes.add(prelude); |
There was a problem hiding this comment.
Optional: this marks the prelude as taken without recording the rule that took it. If the same declaration is compiled twice, StyleX's correct dedupe of the second injection falls through to the branch below and the first rule is inserted again into the test sheet. inserted.add(text); here closes that.
PR Analysis Report🔎 Preview ·
|
meta-cla
Bot
added
the
CLA Signed
There was a problem hiding this comment.
This looks ready to merge: the new runtime injection restores the conditional rules StyleX 0.19.0's dev runtime skips, and the new guard test fails without it and passes with it. Two optional hardening points, since no module reaches either path today:
- Adding
inserted.add(text);besidepreludes.add(prelude);instylexInject.tsstops a first rule compiled in two modules from being copied into the test sheet. - Keying rules with StyleX's own
getSeenRuleKeylogic would also restore a two-condition query nested in a one-condition one, and the 16 later@starting-stylerules that the description's "every rule StyleX generated" covers.
[Automated review]
|
|
||
| export default function stylexTestInject(args: InjectArgs): string { | ||
| const text = inject(args); | ||
| if (text === '' || !CONDITIONAL.test(text) || ONE_CONDITION.test(text)) { |
There was a problem hiding this comment.
Optional: a two-condition query nested in a one-condition one, such as @media (min-width: 100px){@media (hover: hover) and (pointer: fine){…}}, matches ONE_CONDITION on its outer prelude and returns here, but StyleX keys it by both preludes without the selector, so a second rule under that pair is skipped and never restored. No current module nests conditions this way; keying with StyleX's own getSeenRuleKey logic would cover it, along with the later @starting-style rules.
…n prelude StyleX 0.19.0's dev runtime keys a conditional rule whose prelude has more than one parenthesized condition by the prelude alone, so jsdom got only the first rule written under, say, @media (prefers-reduced-motion: reduce) and (pointer: coarse), and a test that read a later one read nothing. vitest.config.ts now names internal/test-utils/src/stylexInject.ts as the babel plugin's runtimeInjection: StyleX's own inject, plus each rule it drops, inserted once into a sheet of its own. Test-only: no StyleX code is patched, and the build keeps every rule as before. It comes out at the StyleX bump to a release that carries meta/stylex#1991, which fixes the key upstream. Red first: the new stylexMultiConditionMedia test (two styles, and one style with two declarations, under one two-condition query) failed with StyleX's own injection; on meta#8226 + meta#8682 + main, Indicator/selectionMotion.test.ts failed 2 of 6. Both pass with it, and the full ui suite on main has the same results (15,951 passing, plus the new test).
The setup caught and dropped any error from inserting a rule StyleX's dev runtime had dropped, so a rule jsdom refused would have gone missing without a trace. It never fired: the full ui suite passes with the catch removed (15,952 tests). A refused rule now throws with the rule's text.
7041cb9 to
5ad4aae
Compare
There was a problem hiding this comment.
Changes requested: the title and description promise every StyleX rule, but later @starting-style rules (16 load with the ui entry points today) and a multi-condition query nested in a one-condition one are still dropped, so a test that reads them still gets nothing. Narrow the title, the description's "holds every rule the build holds" and the two file headers to top-level @media, @supports and @container preludes, or key rules with StyleX's own getSeenRuleKey so every dropped rule comes back.
stylexInject.ts:72now throws on the group last-member rule thatsetup.tsdeliberately refuses, so such a rule under a restored prelude would fail its component's import; skip that marker before inserting.inserted.add(text);besidepreludes.add(prelude);stops a first rule compiled in two modules from being copied into the test sheet.
[Automated review]
| const target = ownSheet(); | ||
| if (target != null && !inserted.has(text)) { | ||
| inserted.add(text); | ||
| target.insertRule(text, target.cssRules.length); |
There was a problem hiding this comment.
internal/test-utils/src/setup.ts makes insertRule throw for any rule containing :nth-last-child(1 of so StyleX's injector skips it. Without the catch, such a rule under a restored prelude now throws out of the component module's top-level inject call and fails every test that imports it (a fixture under @media (hover: hover) and (pointer: fine) fails to import with jsdom does not receive the group last-member rule). Skipping texts that contain that marker before this line keeps the throw for other refusals.
…eys by a prelude alone
Review: StyleX 0.19.0's dev runtime keys a rule by an at-rule's prelude
alone not only under a top-level multi-condition query, but also under one
nested in a one-condition query (`@supports (…) { @media (…) and (…) {…} }`),
under a named container query, and under `@starting-style`; the injection
restored only the first case, so a test that read a later rule under the
others still read nothing.
The injection now keys rules with a copy of StyleX's own getSeenRuleKey,
which StyleX does not export, and restores every later rule whose key is an
at-rule's prelude rather than a selector, once each.
It also skips the group last-member rule, which the test setup refuses on
purpose (its marker now lives in jsdomRefusedRules.ts, shared with
setup.ts): a refusal of that rule threw out of the component's import. Other
refusals still throw. And it records the first rule under a key as present,
so the same rule compiled in two modules is not copied into the test sheet.
vjeux
changed the title
|
All three fixed in e3719ab:
|
There was a problem hiding this comment.
Changes requested, for wording only: the injection now restores every rule StyleX's dev runtime drops by key, and the three earlier points are fixed. Two sentences under "Why this choice" still promise more than the head does: @property --astryx-press-alpha and the five group last-member rules never reach jsdom, and when jsdom refuses a rule StyleX inserts itself, StyleX skips it without a throw. Reword them, for example to "holds every rule StyleX's dev runtime dropped under a prelude-only key, as the build does" and "a restored rule the test DOM refuses throws instead of being skipped".
[Automated review]
There was a problem hiding this comment.
Changes requested: the injection now restores every rule StyleX's dev runtime drops by key, and the earlier wording points are fixed, but its record of restored rules does not survive vi.resetModules().
internal/test-utils/src/stylexInject.ts:71-73keepskeys,presentandsheetin module state thatvi.resetModules()resets while StyleX's own injector keeps its keys, so restored rules are inserted again after each reset (DropdownMenu.ssr.test.tsxends with each of its 6 restored rules four times) and a later rule imported first after a reset is never restored; holding the three onglobalThis(for example underSymbol.for('astryx.stylexTestInject')) and adding a guard case that resets between two fixtures under one prelude fixes both.
[Automated review]
| const keys = new Set<string>(); | ||
| const present = new Set<string>(); | ||
| let sheet: CSSStyleSheet | null = null; |
There was a problem hiding this comment.
This record lives in module state. Vitest inlines this file, so vi.resetModules() starts a fresh copy, while StyleX's inject (from node_modules) keeps its seen keys and sheet. After a reset, every rule restored earlier is inserted again into a new style[data-stylex-test-inject] element (six existing ui files hold duplicates; DropdownMenu.ssr.test.tsx has each of its 6 restored rules four times). And a later rule that comes first after the reset is taken for the rule StyleX kept, so it is never restored.
Keeping the three values on a global fixes both:
const STATE = Symbol.for('astryx.stylexTestInject');
type State = {keys: Set<string>; present: Set<string>; sheet: CSSStyleSheet | null};
const state: State = ((globalThis as {[STATE]?: State})[STATE] ??= {
keys: new Set(),
present: new Set(),
sheet: null,
});A guard case that imports one fixture, calls vi.resetModules(), imports a second under the same prelude and expects the later rule exactly once would cover it.
| // StyleX's runtime injection, plus each rule its dev runtime drops | ||
| // under a prelude with more than one condition (StyleX 0.19.0). |
There was a problem hiding this comment.
Non-blocking: the module also restores rules under named containers, @starting-style and @supports (x: f()), not only multi-condition preludes (28 of the 46 it restores today). Maybe: StyleX's runtime injection, plus each rule its 0.19.0 dev runtime drops because it keys the rule by an at-rule's prelude alone.
…es() CodeBunny on meta#8695: the record of restored rules lived in module state, which vi.resetModules() resets while StyleX's injector keeps its keys, so restored rules were inserted again and a later rule imported first after a reset was lost. The record lives on globalThis; a guard case resets between fixtures under one prelude. Co-authored-by: Cindy Zhang <cindyxz@meta.com>
There was a problem hiding this comment.
Looks ready to merge: the injection's record now lives on globalThis, so a restored rule stays single after vi.resetModules() and a later rule imported first after a reset is restored, and the new reset case fails without the change.
Optional, non-blocking: the comment at vitest.config.ts:40-41 still describes only multi-condition preludes (the module also restores named-container, @starting-style and nested-query rules), packages/core/src/__tests__/stylexDeclarations.ts:11 still says runtimeInjection: true, and the description's "Its three cases" now has a fourth.
[Automated review]
cixzhang
added
the
ci:run
This must merge before the second of #8226 and #8682.
Why
A test that reads a component's styles can find nothing where the build has a rule.
@media (prefers-reduced-motion: reduce) and (pointer: coarse), the test runtime keeps only the first rule written, and a test reading a later one getsundefined. The same holds under such a query nested in a one-condition one, under a named container query, and under@starting-style.@starting-stylerules.Indicator/selectionMotion.test.tsfails 2 of its 6 cases ("expected undefined to be '0s'").What led to it
Making touch components behave like native controls puts more styles behind combined queries, such as reduced motion on a touch screen.
The tests run StyleX 0.19.0's development runtime, which keys each rule it injects and inserts one rule per key.
@media,@supportsor@containerquery is keyed by its prelude plus the rule inside it.The upstream fix, meta/stylex#1991 (merged, not yet released), keys every conditional prelude by the rule inside it.
@starting-stylerules still share one key there.Options and tradeoffs
@starting-stylerules stay dropped after it.Why this choice
The test runtime then holds every rule StyleX's dev runtime dropped under a prelude-only key, as the build does, which is what these tests assume. It does so without touching product code or StyleX, and with a clear point where it comes out.
getSeenRuleKey, which StyleX does not export. It restores each later rule whose key is an at-rule's prelude, once.jsdomRefusedRules.ts, shared by the setup and the injection.How to check it
pnpm exec vitest run --project ui packages/core/src/__tests__/stylexMultiConditionMedia.test.ts. On main it fails, because the later rules read nothing. Its three cases:@starting-stylestyles;Indicator/selectionMotion.test.tspasses 6 of 6 instead of 4.motion/roles.test.ts, fails on main too.DropdownMenuLazySheet,Item.swipeLayer,ItemandResizable/utils, 105 of 105, twice.Evidence
Local gates at e3719ab:
pnpm lint(0 errors) andpnpm check:repo.