Add OnyxStore as a standalone, inert module - #833
Conversation
Introduce lib/OnyxStore.ts: a single listener registry (keyListeners Map<key, Set<listener>>) with subscribe / notifyKey / notifyCollection / getState / hasListenersForKey / clearAll. Built on the existing structural-sharing cache (cache.getCollectionData frozen snapshots). This module is inert — nothing imports it yet. The subscription and notification paths (Onyx.connect, useOnyx, OnyxUtils.notify*) are wired onto it in a later change. Adding it alone has zero behavioral impact. Includes tests/unit/OnyxStoreTest.ts (20 tests) covering exact-key and collection-snapshot routing, ref-equality member skips, hasListenersForKey, clearAll, and listener error isolation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28179abee4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…uring a notify only affect later ones
Co-Authored-By: Claude Code <noreply@anthropic.com>
Co-Authored-By: Claude Code <noreply@anthropic.com>
…evious collection A removed member reads undefined on both sides when partialPreviousCollection is omitted, so the ref-equality skip must only apply when previous actually carries the member. Adds regression test plus getState coverage for both the single-key and collection-key paths. Co-Authored-By: Claude Code <noreply@anthropic.com>
|
@fabioh8010 Could you take a look at the comments above? |
No production call site sets it: collection-batch write paths route members through a single notifyCollection(), so per-member notifyKey calls always want the collection-root routing. Drops the option, the NotifyKeyOptions type, and the now-dead suppression test. Co-Authored-By: Claude Code <noreply@anthropic.com>
| } | ||
| } | ||
|
|
||
| // 2. Collection-level routing. Only fires when the write is to a member key. |
There was a problem hiding this comment.
Not sure if you missed this from my last review. if direct writes to a collection root are an unsupported anti-pattern, why not prevent them at compile time?
diff --git a/lib/OnyxSubscriptionManager.ts b/lib/OnyxSubscriptionManager.ts
index 7e61ec7e..8bc43472 100644
--- a/lib/OnyxSubscriptionManager.ts
+++ b/lib/OnyxSubscriptionManager.ts
@@ -78,7 +78,7 @@ class OnyxSubscriptionManager {
* 2. If key is a collection member, keyListeners.get(collectionKey): collection
* listeners for the parent collection.
*/
- notifyKey<TKey extends OnyxKey>(key: TKey, value: OnyxValue<TKey>): void {
+ notifyKey<TKey extends Exclude<OnyxKey, CollectionKeyBase>>(key: TKey, value: OnyxValue<TKey>): void {
// 1. Exact-key listeners
const exact = this.keyListeners.get(key);
if (exact && exact.size > 0) {
There was a problem hiding this comment.
Sorry, forgot to answer about this one. While it makes sense for external consumers like E/App (if we exported this module), it breaks completely internally because OnyxKey and CollectionKeyBase are both string from library's own perspective – external consumers will augment Onyx types with the correct keys but internally we set them to string, see TypeOptions type. Since they are both string, Exclude<string, string> will resolve to never and any call of this function inside the repo will break.
We shouldn't worry about this though, this module isn't supposed to be exported or used externally by consumers.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
Details
Coming from https://expensify.slack.com/archives/C05LX9D6E07/p1788270903471539.
Adds
OnyxStore, a standalone listener registry that will back Onyx subscriptions in the store-based redesign. Nothing calls it yet, so this PR is inert and changes no behavior.OnyxStorekeeps a singleMap<key, Set<listener>>. Subscribing is aSet.add, unsubscribing aSet.delete, and a write looks up only the affected key's listeners:subscribe(key, listener)registers an exact-key listener (a single key, a collection root, or a collection member) and returns an unsubscribe function.getState(key)reads the eager cache: the frozen collection object for collection keys, the cached value otherwise.notifyKey(key, value)dispatches a single-key write and routes to the parent collection's listeners when the key is a collection member.notifyCollection(key, partial, previous)dispatches a batch update, firing collection listeners once and member listeners only for members whose value reference actually changed.A throwing listener is caught and logged so it can't stop the others. The module depends only on
OnyxCacheandOnyxKeys.The next PR wires the subscription layer onto
OnyxStoreand deletes the old per-subscription bookkeeping. Full context is in the proposal thread.Related Issues
Expensify/App#100271
Linked E/App PR
Expensify/App#100316
Automated Tests
Tests were added for the new module.
Manual Tests
There is nothing to test right now as we are just adding a new module without using it.
Author Checklist
### Related Issuessection above### Linked E/App PRsection above, and verified this change against it (E/App CI passed and manual testing completed)TestssectiontoggleReportand notonIconClick)myBool && <MyComponent />.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Avataris modified, I verified thatAvataris working as expected in all cases)mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari