Repository navigation
[Onyx bump] Bump react-native-onyx to 3.0.119 - Onyx GET asynchronous - #102135
Conversation
|
|
02c7b44 to
f27fb19
Compare
|
I’ll keep it as a draft until the Onyx change is merged and we can properly bump the Onyx version here |
72e9d18 to
a8691eb
Compare
a8691eb to
4eff1c8
Compare
|
@MelvinBot review |
Code reviewThe main risk: both lint rules can be bypassed without any Rule gaps (reads that are not flagged)
Config robustness
Workflow (
Repo conventions (App/CLAUDE.md)
ScopeI reviewed the diff at |
14c55ac to
b2af92b
Compare
TaduJR
left a comment
There was a problem hiding this comment.
Thanks @LukasMod. Sorry for the delay. This one needed in-depth review. One route left, see inline.
PR description:
- Item 3: "Search never returns it" isn't accurate. Inside a Search scope,
useOnyxredirectsreport_keys to the snapshot by prefix, Concierge included. - Item 1: worth mentioning the new cache-miss check in
OnyxSubscriptionManager.getState, which can logCache held no value for a key storage still has. - Item 2: missing the direct-call and component-or-hook checks.
- The test counts are out of date.
Onyx follow-ups Expensify/react-native-onyx#850 (NAB):
getState(undefined)now throws. Checkingcache.getAllKeys().has(key)first inreportCacheMissIfStorageHasValuefixes it.- The cache-miss check marks a collection as reported before storage confirms the miss, and never re-checks the cache afterwards.
?? undefinedinget/multiGetis dead code.
Could you check and raise a PR on Onyx side? cc @fabioh8010 for viz
Also
contributingGuides/philosophies/ONYX-DATA-MANAGEMENT.md:74
Onyx.clear belongs in this list too. A read right after an un-awaited clear returns the old value.
contributingGuides/philosophies/ONYX-DATA-MANAGEMENT.md:80
Worth adding: mutating a read result and then calling Onyx.set with it is skipped as unchanged, so nothing is notified or saved.
| } | ||
|
|
||
| return { | ||
| MemberExpression(node) { |
There was a problem hiding this comment.
The Onyx object used as a value still passes both rules:
const api = {...Onyx};
await api.get(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`);Its only value use in src/ is window.Onyx = Onyx in addUtilsToWindow.ts. Could the rule report any other non-member use outside tests/?
There was a problem hiding this comment.
Fixed in b69f95d. Any use of the Onyx object other than a member access now reports in every file outside test code: spread, alias, argument, return, export, re-export and dynamic keys like Onyx[name]. window.Onyx = Onyx is the one allowed shape, so addUtilsToWindow stays as is. 17fe7b0 makes a blanket disable over such a line count as a bypass. Hope it resolves those edge case scenario. I added tests to cover that too
|
This PR adds a new |
|
@TaduJR Thanks! The description is updated. Please check whether the reasoning for Item 3 works for you. This is what I confirmed with Carlos (@luacmartins): Search should never return that chat. It's primarily used for handlers and is a frequently changing candidate. I run extra sessions to look for some exceptions and also updated the comments and JSDoc to make the lint work easier to understand. I’ll validate those Onyx follow-ups separately. |
|
Everyone has approved, or had previously approved. Merging! |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 chuckdries has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/chuckdries in version: 9.5.21-0 🚀
|
|
No help site update needed. This PR only bumps |
|
🚀 Deployed to staging by https://github.com/chuckdries in version: 9.5.23-0 🚀
|
|
No help site update needed. This PR only bumps |
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.5.23-3 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
react-native-onyxfrom3.0.117to3.0.119. The bump contains 2 Onyx PRs:UnknownError: Internal error.(fix(storage): reopen the IDB connection onUnknownError: Internal error.react-native-onyx#853). Onyx classifiedUnknownError: Internal error.as an unclassifiable error, so the operation layer retried each write six times and gave up, while the connection layer never reopened the database. This maps that exact wording to the class the connection layer already owns, so a failed read or write now drops the cached connection, reopens it, and retries (budgeted at 3 attempts, reset on any success). If the reopen does not help, the existing heal budget exhausted alert fires and the write is dropped, instead of every operation burning its full retry budget.Onyx.get(key)andOnyx.multiGet(keys)become public and typed (Add public Onyx.get - mutable react-native-onyx#850). No app code reads with them yet. The only app change the bump needs issrc/setup/addUtilsToWindow.tsandreact-native-onyx.d.tsdropping the hand-madeconnectWithoutViewshim and itsWindowdeclaration (16 lines), so the dev consoleOnyx.getandOnyx.loggo through the library method. 3.0.119 also adds a cache-miss check toOnyxSubscriptionManager.getState, which backsuseOnyx,Onyx.connect,Onyx.getandOnyx.multiGet: when a non-collection key that storage lists is missing from the cache, Onyx reads storage and, if a value is there, logs ahmmm[Onyx] Cache held no value for a key storage still has: <key>once per collection per session; it only logs and still returnsundefined.errorrepo-wide.rulesdir/no-unsafe-onyx-readdecides where a read may run: only insrc/components,src/pages,src/hooksandtests, inside a component or hook body or a function nested in one, and never during render, at module scope or inside an effect. A same-file helper that render calls counts as render. Such a read returns stale data or re-runs on every render, so the rule cannot be disabled.Onyx.getandOnyx.multiGetmust be called directly, and theOnyxobject may only be used through a member:Onyx.get.call,keys.map(Onyx.get),const read = Onyx.get,const {get} = Onyx,{...Onyx},Onyx[name]or a re-export is reported in every file outside test code, since it hides the key and the call site from lint.window.Onyx = Onyxfor the dev console is the one allowed value use.rulesdir/no-onyx-get-snapshot-keydecides which keys may be read. It bans the Search snapshot keys (SEARCH_SNAPSHOT_ONYX_KEYS) and any key lint cannot resolve, including eachmultiGetelement, becauseuseOnyxredirects those keys to a Search snapshot andOnyx.getwould return live data where the component saw the snapshot. Some reads are known safe, such as a migrated module-levelOnyx.connector auseOnyxWithoutSnapshotssubscription, so this rule can be disabled with a mandatory reason.useOnyxredirects it to the snapshot like any otherreport_key, but Search never returns the Concierge chat, so no snapshot holds it and the livereport_entry is the only place it exists. The exemption applies only to${ONYXKEYS.COLLECTION.REPORT}${id}whereidis a same-fileconstread fromONYXKEYS.CONCIERGE_REPORT_ID.scripts/onyxLintBypass.ts(renamed fromonyxConnectBypass.ts) fails lint on any inline disable ofno-onyx-connect,no-unsafe-onyx-reador the OnyxUtils import restriction. It also fails on anyeslint.seatbelt.tsvrow forno-unsafe-onyx-readorno-onyx-get-snapshot-key, and it counts a blanket disable over a by-reference use or a value use of theOnyxobject, not only over a call.scripts/onyxLintDisableReason.tsaccepts a disable ofno-onyx-get-snapshot-keyonly in the-lineor-next-lineform with at least 12 letters or digits after--.ONYX-DATA-MANAGEMENT.mdlistsOnyx.getandOnyx.multiGetas read paths, names when a live read is right (a migrated module-levelOnyx.connectused only by handlers), and shows the disable shape, including where it sits for amultiGet..github/workflows/onyxGetReviewers.ymlrequests the Onyx performance reviewers and leaves a comment whenever a non-draft PR adds a net-newOnyx.getorOnyx.multiGetcall undersrc/, ignoring mocks, comment lines and the dev console'swindow.Onyx.get. It mirrorsonyxConnectWithoutViewReviewers.yml..github/workflows/lint.ymladdseslint-plugin-local-rules/**,src/libs/SnapshotUtils.tsandsrc/CONST/runtimeDefaults.tsto the ESLint cache key, since ESLint's own cache hashes the config without rule functions, so a rule edit alone never invalidated it.Automated coverage:
tests/unit/NoUnsafeOnyxReadRuleTest.ts(294 cases),tests/unit/NoOnyxGetSnapshotKeyRuleTest.ts(101, including an exhaustive check that the rule rejects exactly theONYXKEYSentriesuseOnyxwould redirect),tests/unit/OnyxLintBypassTest.ts(88),tests/unit/OnyxLintDisableReasonTest.ts(13) andtests/unit/SnapshotUtilsTest.ts(2, the prefix checkuseOnyxand the rule now share).Fixed Issues
$ #101910
PROPOSAL:
Tests
Onyx 3.0.118
The steps below cover Onyx #853 (reopen the IDB connection on
UnknownError: Internal error.).Test 1: Data loads and persists after the IndexedDB database is deleted (web only)
indexedDB.deleteDatabase('OnyxDB');.(await import('/node_modules/idb-keyval/dist/index.js')).get('test')in the console and confirm it resolves without an error.Onyx 3.0.119
The steps below cover the public
Onyx.getandOnyx.multiGetAPI, the two lint rules with their scripts, and the reviewers workflow. No extra code is needed.Test 2: Dev console helpers go through the library method (no other usage currently)
await Onyx.get('session')in the console and confirm it returns the session object.Onyx.log('session')and confirm the session is logged.Offline tests
QA Steps
Test 1: Sign in, send a message, sign out and back in (all platforms)
Test 2: Two web tabs
Prerequisite: signed in on Web in Tab A and Tab B.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.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.native.mov
Android: mWeb Chrome
android.web.mov
iOS: Native
ios.native.mov
iOS: mWeb Safari
ios.web.mov
MacOS: Chrome / Safari
TEST 1:
onyxget-web-1.mov
TEST 2:
onyxget-web-2.mov
web.mov
web.2.tabs.mov