fix(native): stop a read swallowing a notification, and share reactivity state across a dual-package split - #427
Open
YevheniiKotyrlo wants to merge 7 commits into
Conversation
`observable()` used `value` for two jobs: the cache a reader gets back, and the yardstick `run()`/`set()` compare against to decide whether observers still need telling. `get()` refreshes the cache, so a read that landed between a write and its notification moved the yardstick onto the new value and the change was then treated as already delivered - the subscriber was never notified. Two interleavings reach this through the public API with no timing involved. Inside a batch, a write queues the derived observable's effect and any read before the flush advances the cache, so the flush finds them equal and returns. Unbatched, a co-observer registered on the source ahead of the derived observable's own effect reads the derived value during the fan-out with the same result. Track the published value separately: only `notify()` advances it, so a read can no longer cancel a notification. `get()` publishes when nobody is subscribed, since no observer can be owed a value produced while the observer set was empty - without that, the first dependency change would notify for a value the subscriber had already read. `set()` collapses to a single guard. The static branch's comparison was provably identical to the dynamic one (a static observable has `didInit` from construction, so `get()` never refreshes its cache and the two values cannot diverge), and two guards meant one of them could never be made to fail. The test drives every gap a read can land in around a write, batched and unbatched, over a write sequence that returns to an already-seen value and includes a write that changes nothing. It asserts both halves: a settled write leaves no subscriber stale, and no read manufactures a notification.
`package.json`'s `exports` map sends `import` to `dist/module/**` and `require` to `dist/commonjs/**`. Metro resolves per requesting module, so one app can evaluate this module twice, and every piece of state it owns was duplicated by that: two `Dimensions` listeners writing two `vw`s, two `colorScheme`s, two container families, and two `observableBatch`es. A subscriber registered through one copy never heard a write made through the other, and a batch opened by `StyleCollection.inject` on one copy did not capture writes made through the other. The state is pinned as one object rather than a global per export, matching `style-collection.ts`. The pieces are mutually coupled - `vw` and `vh` derive from `dimensions`, and the listener that writes them does so through `observableBatch` - so a per-export guard invites a later edit that shares some and not others, and a half-shared graph is harder to diagnose than an unshared one. Building it in one initializer also makes the `Dimensions` and `Appearance` registrations part of what runs once. The pure exports stay unpinned: `observable`, `family`, `weakFamily` and `cleanupEffect` close over no process state, so a second copy of them is harmless. `VAR_SYMBOL` is interned by `Symbol.for` already. The test reproduces the hazard rather than describing it: two `jest.resetModules()` + `import()` pairs evaluate the source twice against one `globalThis`, which is the same shape as the two builds. It asserts the two copies are genuinely distinct first - `observable` differs between them - so the sharing assertions cannot pass by the module simply being cached. The state census is read off the guarded object, so state added later is covered without editing the test.
`* { --x: ... }` compiles to `vu` and `:root { --x: ... }` compiles to
`vr`, and the resolver consults `universalVariables` before
`rootVariables`. The injector wrote both into `rootVariables`, so
`universalVariables` was never populated and the resolver's universal
branch was dead.
That is not only dead code. `set` replaces a variable's whole value list,
and the `vu` loop runs after the `vr` loop, so a universal declaration
overwrote the root declaration of the same name. When the universal
declaration sits behind a media query that does not match, it resolves to
nothing and the root value it replaced is gone:
:root { --my-var: #123456; }
@media (min-width: 99999px) { * { --my-var: #abcdef; } }
resolved to no colour at all instead of falling back to `#123456`.
Injecting `vu` into `universalVariables` keeps the two censuses apart, so
each resolves independently and the resolver's existing order decides
between them.
That order is deliberate and now asserted: `*` outranks `:root` for any
non-root element, because a `*` declaration applies to the element
directly while a `:root` declaration only reaches it by inheritance. It
held before only because one census was always empty, so swapping the two
reads broke nothing. The tests cover both directions of the fallback and
both rankings, and a swap of the resolver's two reads now fails.
The harness cleared `StyleCollection.styles` between tests but left the `:root` and `*` variable families alone. Both are process-global, so a `vr` or `vu` entry injected by one test resolved in every test after it. Injecting universal variables into their own family makes this worse, since there are now two registries carrying state nobody clears. `resetGlobalVariables` clears both and re-applies the variables the runtime declares for itself. Those cannot simply survive the clear - `__rn-css-rem` backs every relative length, so dropping it would resolve every `em` and `rem` to nothing - so seeding moves into a function that both module init and the reset call. No existing test changes behaviour: 1097 passing before, 1097 after, with the same three pre-existing babel failures. The tests drive the reset directly rather than relying on a previous test having dirtied the registries, so each passes alone and in any order. The end-to-end case declares its variable twice, once behind a media query that cannot match. A `:root` variable with a single declaration is folded into the rule by the compiler, so a test written the obvious way never reaches the registry at all and holds whatever the reset does.
A `:root` custom property with exactly one declaration is folded straight into the consuming rule by the compiler - the stylesheet carries no `vr` entry at all, and `color: var(--my-var)` compiles to `color: #123456`. A test written that way asserts the compiler's constant folding and never reaches `rootVariables`, so it holds whatever the registry does. Declaring the variable a second time behind a media query that cannot match keeps it dynamic. Removing the resolver's `rootVariables` branch now fails this test; before, it passed.
`containerHeightFamily` projected `.width` off the shared layout cell, so every height-based container query answered with the width — `@container (height >= 400px)` matched a 500x100 box, and `(orientation)` reported portrait for every real container because `width > height` was unsatisfiable. A coverage sweep found the family's body never executed, which is what let a copy-paste of its width sibling sit there: no test read either axis. Three cases over a deliberately NON-square fixture, because a square probe makes the two axes agree and the wrong field answers plausibly — both axes from one layout, both re-answering when it changes, and both zero before it is measured. Reinstating `.width` fails two of them.
`Dimensions.addEventListener`'s callback is the only path a rotation takes to `vw` and `vh`, and no test executed it — the same shape that let the container height axis read `.width`. Its batch is the part worth pinning: without it the two `set` calls notify separately, so an effect reading both runs once against the new width beside the old height. One case asserts both axes update and a subscriber reading both runs exactly once; dropping the batch makes it 2. A second asserts the batch is closed again afterwards, because one left open swallows every later notification in the process; removing the reset reddens it.
Contributor
Author
Device evidence — before / afterUNFIXED — The first two bars are red. FIXED — Only the second bar is red. The height query reads the host's height — 24, which does not clear 60 — so the rule is withheld and the bar matches the no-container baseline.
Read the The second and third bars are the anti-vacuity pair, and they do not move. The second is red in both frames, so the container registered and size queries resolve at all; the third sits under no container and is blue in both, so a subject with nothing above it was never matching. Only the first — the height query — flips. That is also why a square host would have hidden this indefinitely: with the two axes equal, the wrong field returns a plausible number and every assertion downstream passes on it. Every other container case in the gallery used one, which is why nothing caught it. Measured rather than asserted: 72 of the two frames' 2400 pixel rows differ, and they are rows 1308–1379 — exactly the bar band Two more consumers move with it, neither visible here: Both frames: pooled Android 16 emulator, 1140×2400 @ 480dpi, light scheme, same session. |
This branch has not been deployed
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
1 participant
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.
Problem
Six fixes to the native reactivity primitive and the registries built on it.
A read could swallow an observable notification. The cache advanced past the value a subsequent
run()'s guard would read, so a subscriber that read at the wrong moment never learned the value had changed. This is the primitive everydark:,hover:and container-query rule is answered from, so a swallowed notification is a rule that silently stops re-evaluating.reactivity.tshad no dual-package guard at all. Its process-global state is now pinned toglobalThis, so two copies of the module — the CJS and ESM builds loaded by different consumers — share one registry rather than each keeping a private one that the other's writes never reach.universalVariableswas created and exported but never written. It now injects into its own family. Measured before fixing: universal correctly outranks root, so this was dead code plus an accidental ordering rather than a wrong value — the fix makes the intent real rather than repairing a live wrong answer.The jest harness leaked root and universal variables between tests.
beforeEachclearedStyleCollection.stylesbut notrootVariablesoruniversalVariables, so avrentry survived into the next test. That is a harness defect that makes other people's tests pass for the wrong reason, and it already forced one:roottest into a file of its own.A
:rootvariable test now reaches the runtime registry rather than asserting only the compiled shape.containerHeightFamilyprojected.widthoff the shared layout cell, so every height-based container query answered with the container's width.getContainerFeatureValuehas three size consumers and all three are wrong, none loudly:height@container (height >= 400px)matches a 500x100 boxorientationwidth > widthportrait, unsatisfiably — no container is ever landscapeaspect-ratiowidth / width1, whatever the box isA coverage sweep is what found it: the family's body never executed under any test, which is what let a copy-paste of its width sibling sit there unnoticed.
Normatively the axis question is CSS Conditional 5 §6.1.2 —
heightis the container's own block-axis size, and §6.1.5 / §6.1.6 deriveaspect-ratioandorientationfrom both axes, which is why one swapped field reaches three features.Solution
Six commits, one per defect above, in that order.
The read no longer advances the cache past the value a later
run()guard reads.reactivity.ts's process-global state is pinned toglobalThis.universalVariablesinjects into its own family. The jest harness clears root and universal variables between tests. A:roottest reaches the runtime registry rather than the compiled shape. AndcontainerHeightFamilyreads.height.The last one is a one-character change with three consumers behind it, which is why it is the last commit rather than the smallest.
Tests
21 test declarations across four files, three of them new:
reactivity-dual-package.test.ts(132 lines),root-variable-reset.test.tsx(66),universal-variables.test.tsx(90), plus 331 lines inreactivity.test.ts.What makes the axis cases non-vacuous is a deliberately non-square fixture,
{ width: 500, height: 100 }— a square probe makes the two axes agree, which is why nothing caught the defect for as long as it stood. Mutation-proved: reinstating?.widthturns exactly two of the three red (width reads width and height reads height,each axis re-answers when the layout changes) and leavesan unmeasured container answers zero on both axesgreen, because zero is zero on either field.The window-resize cases are pinned the same way: dropping the batch in
Dimensions.addEventListenermakes a both-axis subscriber run twice instead of once, and failing to close the batch afterwards swallows every later notification in the process. Both were measured by making the change and watching the case go red.Verification
Each fix has a red-then-green test. Fix 1's is the interesting one: it drives the primitive directly rather than through a rule, because a notification swallowed inside a rule evaluation is indistinguishable from a rule that legitimately did not match.
Fix 6's three cases run over a deliberately NON-SQUARE fixture (
{ width: 500, height: 100 }), and that is the whole reason they can fail. A square probe makes the two axes agree, so the wrong field answers plausibly and every assertion downstream passes on it — which is exactly why nothing caught this for as long as it stood. The three read both axes from one layout, re-read both when it changes, and read both as zero before it is measured. Reinstating.widthfails two of them.The same coverage sweep that found fix 6 found one more path nothing executed, and it is covered here without a behaviour change:
Dimensions.addEventListener's callback, which is the only route a rotation takes intovwandvh. Its batch is what stops a subscriber seeing a half-updated viewport — without it the twosetcalls notify separately, so an effect reading both runs once against the new width beside the old height. Two cases pin it: dropping the batch makes a both-axis subscriber run twice, and failing to close it again swallows every later notification in the process.Fix 4 is worth calling out as a harness change rather than a product change — it makes some existing tests stricter, and any that were relying on the leak are now honest.
Suite on this branch:
3 failed, 21 skipped, 1105 passed, 1129 total.yarn typecheckandyarn lintexit 0. The 3 are the known Windowsbabel-plugin-testerbaseline (an unrewritten relativerequire("../View")), present onmainand unrelated.Base
Branched off
f70c402.mainhas since taken #451 (a5002c5). 4 of the 8 files this changes also moved there, and 2 genuinely conflict —src/__tests__/native/universal-variables.test.tsx,src/native/reactivity.ts. Every measurement above was taken onf70c402. Say the word and I will re-apply it onto currentmain.