Skip to content

[$1500] Expose Onyx.get #101910

Open
Open

Description

@chuckdries

Coming from #expert-contributors

Problem

Some components subscribe to Onyx values purely so they can pass them to event handlers and side effect functions. When a component subscribes to an Onyx value it never uses for rendering UI, a change to that value re-renders the component without changing anything on screen, which is unnecessary work.

Solution

Expose an Onyx.get() function that can lazily read values from the store

  1. Ship Onyx.get() in react-native-onyx. Onyx.get(key: TKey): Promise<ReadonlyDeep<OnyxValue>> on the public object, implemented on tryGetCachedValue wrapped in afterInit. That is the read useOnyx already performs, and it handles whole collections..
  2. Define where it may be called. Handlers, network callbacks, actions and library helpers whose return value no caller consumes synchronously, useCallback and useEffect bodies among them, since both run on an event. Never a render body, a useOnyx selector, a useMemo body, a useState or useReducer lazy initializer, a useSyncExternalStore snapshot or module scope..
  3. Ban the Search snapshot keys. Our useOnyx wrapper redirects those keys and Onyx.get does not, so it always returns live data and a converted subscription silently changes what a Search-scoped component sees. Scope membership is a runtime property of the mounted tree, so rather than try to decide it, a new lint rule refuses all seven search-overridden keys outright. The ban follows from the redirect, not from anything about the keys themselves. If a future initiative removes snapshot_ and useOnyx wrapper, the rule can be lifted and these high-traffic keys converted..
  4. Enforce it. New lint rule no-unsafe-onyx-read takes every misuse that one file's syntax decides, tsc takes the forgotten await and any attempt to mutate what comes back, and ONYX-1 review takes the rest. Broken down in the thread details..
  5. Prove it on one pilot. useSwitchToDelegator is a great candidate because it has 8 useOnyx subscriptions, none of which are used for rendering. PoC commit.
  6. Convert the easy cases in small PRs. 248 bindings whose value is only used at event time and needs no other change, across 123 files, batched into 19 independent PRs. Details in the thread..
Issue OwnerCurrent Issue Owner: @TaduJR

Activity

  1. chuckdries commented on Sep 22, 2026

    @chuckdries
    ContributorAuthor

    We may also want to do something like https://github.com/Expensify/App/blob/e9cc9d0c696597bf151fa877f7ac74f68c60a18b/.github/workflows/onyxConnectWithoutViewReviewers.yml where we ping a set of reviewers each time a new usage gets added

  2. fabioh8010 commented on Sep 23, 2026

    @fabioh8010
    Contributor

    Hey @chuckdries @tgolen @roryabraham @mountiny based on the lengthy discussion we had in the thread, @LukasMod and I came back with a revised rollout plan so we are all aligned before the next steps.

    Phase 0 — API only

    • Ship Onyx.get without the ReadonlyDeep type for now. Why?
      • In the E/App PR, which has zero migrations, using Onyx.get only in some test files already required ~50 signature changes in ReportUtils plus six other lib files. Any ReadonlyDeep value passed into a lib function typed with our current Onyx types requires changing that function's signature. Every handler in Phase 1 that passes a read value into a lib helper would hit the same thing, so we can't keep readonly and also promise "no lib changes" in the first Phases. I vote for removing it for now and revisiting it per key once we have real usages of Onyx.get in the codebase.
    • In the E/App PR we ship the Onyx bump, no-unsafe-onyx-read, ONYX-1, the docs and the addUtilsToWindow cleanup, with three small additions to the lint rule:
      • Onyx.get is allowed only under src/components, src/pages, src/hooks and tests. Everywhere else is an error.
      • A read inside an effect body is an error for now (see Phase 2).
      • scripts/checkOnyxConnectBypass.ts also covers no-unsafe-onyx-read, so an inline eslint-disable fails CI.
    • In the same PR we also ban OnyxUtils.get too since we have a proper API now.

    Phase 1 — drop-in group, components and hooks

    • The initial 19 PRs described in the proposal, minus reads that would land inside an effect and minus conversions that force a sync consumer to become async (those go to Phase 2 or 3, see below).
    • Every PR is the same edit: delete the useOnyx line and await Onyx.get(key) in the handler that used it. We can start with useSwitchToDelegator, which already has tests written before the conversion. We need to regenerate this list first though, since it was made before the proposal.
    • When these PRs are done we stop and review before going further. We will look at three things: how many useOnyx bindings were removed, whether any of the converted flows regressed, and how many of the ONYX-1 review comments caught real problems vs false alarms (this tells us how much maintenance the new API actually adds). Only then we decide if we continue to Phase 2.

    Phase 2 — the "needs a fix" group and lib replacements
    On the component side, this is the useOnyx usages that are not a straightforward change:

    • Reads inside useEffect. A useOnyx usage can be replaced by Onyx.get inside an effect only if the Onyx value was not in that effect's dependency array. If it was, the subscription is what re-runs the effect and it stays. This is visible in the diff, so it can be a CI check instead of a review judgment.
    • useOnyx calls that use a selector. Onyx.get has no selector option and returns the raw value, so the conversion has to call the selector on the result.
    • Chained keys, where the value of one useOnyx is used to build the key of the next one.
    • Handlers that are consumed synchronously by something that cannot await. These convert in Phase 2 only when the caller can await without changing its own signature. If a chain of callers would have to become async, it goes to Phase 3.

    On the lib side, this is where connectWithoutView gets reduced. Today ~91 of the ~195 connectWithoutView calls only exist to keep a module variable up to date so a function can read it later, nothing reacts to the change. Those can become an await Onyx.get(key) inside that function, but only when the function is already async, or when it returns nothing that a caller uses, so callers can keep calling it without changes. Same rule as above: if callers would have to become async, it goes to Phase 3. To keep this controlled, a lib file only gets added to the allowlist in a PR that also removes a connectWithoutView from that same file, reviewed by the same group that reviews connectWithoutView today. Every approval removes a subscription instead of adding one.

    Phase 3 — reads moving into actions (follow-up work/proposal)

    • The original maintainability idea, e.g. navigateToConciergeChat from 26 call sites to 1, plus every conversion Phases 1 and 2 skipped because it would make a chain of callers async, whether it started as a useOnyx in a component or a connectWithoutView in a lib.
    • Reads only in action creators under src/libs/actions, never in utils, with the render-reachability check running in CI for every converted function.
    • Separate follow-up proposal, with data on how far async actually spreads.
  3. tgolen commented on Sep 23, 2026

    @tgolen
    Contributor

    I'd like to make a small request on the rollout plan. Could we please put the enforcement into place first, before introducing .get() into the code?

  4. LukasMod commented on Sep 24, 2026

    @LukasMod
    Contributor

    I prepared 2 PRs based on @tgolen suggestion and the Phase 0 plan. I dropped the readonly changes to avoid having multiple signatures at this stage.

    I also added one extra thing: Onyx.multiGet for reading multiple keys, which should prevent repetitive Promise.all blocks. Looking back at some previous PoCs, I often migrated multiple keys in a single handler, so it would be nice to have a ready-made method for those cases. WDYT about the idea and the name?

    rn-onyx: Expensify/react-native-onyx#850

    • removed the readonly changes
    • introduced public get / multiGet

    E/App: #102135

    • linked the rn-onyx PR
    • removed the readonly signature changes
    • removed the OnyxUtils.get swap from tests
    • kept only the addUtilsToWindow change, to resolve the get() override conflict
    • added the lint rule, AI reviewer rule, and docs
  5. mountiny commented on Sep 24, 2026

    @mountiny
    Contributor

    Thanks for the update! Lets try to get on C+ for this

  6. changed the title [-]Expose Onyx.get[/-] [+][$1500] Expose Onyx.get[/+] on Sep 24, 2026
  7. melvin-bot commented on Sep 24, 2026

    @melvin-bot

    ⚠️ Could not update price automatically because there is no linked Upwork Job ID. The BZ team member will need to update the price manually in Upwork.

  8. mountiny commented on Sep 24, 2026

    @mountiny
    Contributor

    @LukasMod @fabioh8010 - @aimane-chnaif will be C+ and reviewer on all the prs for this project. Please make sure they are tagged and added on all the prs as I think onyx prs will still autoassign randomly. Also feel free to start threads in Slack for PRs for easier communication so we can deliver this smoothly and sooner than later.

    Thanks!

  9. tgolen commented on Sep 24, 2026

    @tgolen
    Contributor

    I like the addition of multiGet() 👍

  10. LukasMod commented on Sep 25, 2026

    @LukasMod
    Contributor

    On the EApp PR, I addressed the feedback and added a script to auto-assign reviewers for all Onyx.get/multiGet usage, the same way we do for connectWithoutView, as requested. For now, I assigned @tgolen, @mountiny, @chuckdries.

    The way I see it now:

    • Onyx PR first - ready for review. @mountiny MelvinBot ignored my comment there, so I can't assign @aimane-chnaif myself
    • Bump the Onyx version in the EApp PR - I’ll keep it as a draft until we bump the Onyx version, but it can be reviewed in the meantime.
    • Replace OnyxUtils and the remaining window.Onyx.get usage in a separate clean up PR

    End of Phase 0

    • I’ll be OOO from 28.09 to 01.10. Someone from the team will pick this up while I’m away
  11. fabioh8010 commented on Sep 28, 2026

    @fabioh8010
    Contributor

    I'm taking over from @LukasMod here. Applied some review suggestions on Expensify/react-native-onyx#850, will continue working there.

  12. fabioh8010 commented on Sep 29, 2026

    @fabioh8010
    Contributor

    Updates:

    • Addressed comments in both PRs, updated them with main.
  13. LukasMod commented on Oct 2, 2026

    @LukasMod
    Contributor

    Updates:

    • addressed latest comments
  14. self-assigned this
    on Oct 21, 2026
  15. melvin-bot commented on Oct 21, 2026

    @melvin-bot

    @TaduJR — to hire @TaduJR on Upwork, add the External label so a job is created and the offer is sent automatically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

ImprovementItem broken or needs improvement.ReviewingHas a PR in reviewWeeklyKSv2

Type

No type

Projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions