Repository navigation
Share selector work between useOnyxState hooks and add createDerivedValue - #7
Draft
BartekObudzinski wants to merge 2 commits into
Draft
BartekObudzinski wants to merge 2 commits into
BartekObudzinski wants to merge 2 commits into
Conversation
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.
Details
useOnyxStatekeeps its last result per hook instance, so ten components reading the same derived value compute it ten times. This PR adds two ways to share that work.useOnyxStateshares a stable selector's result between instances. When several components pass the same module-level selector over the same dependency references, it runs once and the rest reuse its result. Inline selectors are new functions on every render, so they behave as before. A run that readpreviousState, or saw it missing, is never shared, because it depends on its own instance.createDerivedValue({dependencies, compute})defines a value at module level that is computed once per change for every hook that reads it, including inline and per-ID selectors like(state) => reportNames.get(state)[reportID].computegets the previous dependencies and result, so an expensive value can update only what changed.previous.keep()leaves everything as it was, for a compute that has to wait (say for translations) or that failed, so the next read tries again.Onyx.cleardrops both caches, so nothing from a signed-out account stays in memory.Selector runs per 10 writes in a Jest harness with 9,000 cards and all card list consumers mounted:
useOnyxState, no sharingI also moved the report attributes cache from the
useOnyxStateport ofREPORT_ATTRIBUTESontocreateDerivedValue(local only, not part of this PR). Its hand-written module cache becomes thecomputefunction, the existing tests pass, and the work stays the same with 5,000 reports: one compute per change, 5,000 report names on a start without a snapshot, none with one.These change how much work runs, not timing. I could not get clean timing numbers on my machine.
Related Issues
Expensify#862
Linked E/App PR
None yet. This targets the
useOnyxStatebranch, which has no E/App PR pinned to it. The report attributes port was tested locally against a build of this branch.Automated Tests
tests/unit/useOnyxStateTest.tsx, "sharing results between instances":previousStateis not shared.previousStateis not handed to an instance that has one.tests/unit/createDerivedValueTest.tsx:computegets the previous dependencies and result and updates only the changed member.keep, every read runs again and the next compute diffs against the last real one.reset, also called from insidecompute, starts from scratch.Onyx.clearstarts it from scratch.Manual Tests
npm run build) and copydistinto an App checkout on theuseOnyxStatebranch.createDerivedValueand run its tests, then count computes with and without the change.Not run on devices yet.
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
🤖 Generated with Claude Code