fix(native): stop a read swallowing a notification, and share reactivity state across a dual-package split - #427
Open
YevheniiKotyrlo wants to merge 5 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.
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.
Five fixes to the native reactivity primitive and the registries built on it.
The defects
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.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.:rootvariable test now reaches the runtime registry rather than asserting only the compiled shape.Evidence
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 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.
yarn typecheckandyarn lintexit 0. The 3 remaining failures are the known Windowsbabel-plugin-testerbaseline (an unrewritten relativerequire("../View")), present onmainand unrelated.