-
Notifications
You must be signed in to change notification settings - Fork 96
[POC] useOnyx subscribed #808
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
LukasMod
wants to merge
11
commits into
Expensify:main
Choose a base branch
from
callstack-internal:feat/useonyx-subscribed
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+275
−9
Draft
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
dd67718
Add subscribed option
LukasMod f4bd80c
update docs
LukasMod 76830e4
restore import order
LukasMod 9e6e74e
prettier fix
LukasMod 9b8bcea
remove subscribed guard on subscribedRef
LukasMod 52c7e8f
fix initial edge case
LukasMod e12bd86
use useLiveRef for subscribedRef
LukasMod b51368c
restore useEffect
LukasMod 13f825e
update unit tests
LukasMod c034d60
add Catch-up for the commit effect gap
LukasMod 578edad
update tests and comments
LukasMod File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
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
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a consumer flips
subscribedfromtruetofalse(for example during a screen-blur render), this passive effect leavessubscribedRef.currentat the oldtruevalue until after commit. Any Onyx write from a sibling layout effect or immediate microtask in that commit→effect window will pass the guard in the connection callback and callonStoreChange(), causing the off-screen/background re-render this option is meant to suppress. Syncing the ref during render or in a layout effect avoids that stale-true window.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right that there's a small gap. When subscribed flips from true to false, the ref only updates after React runs effects. If an Onyx write lands in that tiny window between the render committing and the effect running, the callback still sees true and triggers one re-render that ideally would have been skipped.
We're keeping it this way on purpose, for three reasons:
First, the damage is tiny and harmless. Worst case is one extra render, showing correct data, on a screen that just went off-screen. Compare that to the opposite direction (false to true): if the ref were stale there, a visible screen would miss fresh data, which is a real correctness bug. That direction is already handled by the catch-up in this effect. So the rule of thumb here is: when the gate is briefly wrong, it should fail by rendering too much, never by showing stale data. This window fails in the safe direction.
Second, the suggested fixes are worse than the problem. We can't "catch up" after the fact like we did for the focus direction, because you can't undo a render that already happened. The only real fix is updating the ref earlier. Updating it during render was already flagged as a P2 in an earlier round: with concurrent rendering, an aborted render could write false into the ref while the visible screen is still subscribed, and then a visible screen stops getting updates. useLayoutEffect would work but brings the SSR warning problem and would be the first use of that pattern in this library. Neither trade is worth it to skip one background render in case of that tiny gap.
Third, TanStack Query has exactly the same window in its subscribed option and ships with it unmitigated. Their subscription is only torn down when React runs the passive effect, so a query update in that same commit-to-effect gap also re-renders the component that just unsubscribed. This isn't an implementation bug on our side, it's just how useSyncExternalStore works: subscription changes always take effect when effects flush, not when the render commits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think it's not worth investing on this, bringing more unecessary complexity is exactly what we want to avoid in the hook