Closed chrispader closed 3 months ago
Requesting a review from myself so this doesn't get lost, but planning to let Janic review first.
This PR seems to be the fix for a regression in https://github.com/Expensify/App/pull/42057, which was reverted in https://github.com/Expensify/App/pull/42725
@neil-marcellini @janicduplessis i'm gonna prioritize this PR right now so we can hopefully merge this today. I'll let you guys know when i'm done with the implementation so you can review
cc @mountiny
@neil-marcellini @mountiny @janicduplessis this PR is now ready for review! Once we merged this, we can review, QA and merge https://github.com/Expensify/App/pull/42772
@mountiny addressed all your comments and improved a few other parts of the implementation
🚀Published to npm in v2.0.43
Coming from this issue https://github.com/Expensify/App/issues/42529. useOnyx
in useReportIDs.tsx here are sent reportActions
with null
values to the E/App. I see this PR remove null values from cache and I test the issue with this fix in v2.0.43
and it fixed.
@chrispader I just need to confirm if this PR fix the above issue because its title indicates it is for improve performance
Coming from this issue Expensify/App#42529.
useOnyx
in useReportIDs.tsx here are sentreportActions
withnull
values to the E/App. I see this PR remove null values from cache and I test the issue with this fix inv2.0.43
and it fixed.@chrispader I just need to confirm if this PR fix the above issue because its title indicates it is for improve performance
@ahmedGaber93 this PR is also supposed to remove null
values from all getter options, like withOnyx
, useOnyx
and Onyx.connect
. If this PR fixed your issue, it should be fine!
Coming from this issue https://github.com/Expensify/App/issues/42529. E/App still receive null values in Onyx.connet
here. For reproduce steps see issue description. I think we can add an extra null check in E/App to urgent fix, but this is still needing to fix.
Coming from this issue Expensify/App#42529. E/App still receive null values in
Onyx.connet
here. For reproduce steps see issue description. I think we can add an extra null check in E/App to urgent fix, but this is still needing to fix.
I couldn't reproduce this myself, but i suppose this stems from old data (with null
values) still being in storage, which is then returned.
We only remove null
values on write, so unless a key doesn't change in the meantime, it will still hold the null
value
@ahmedGaber93 could you link me to the exact repro steps? the ones from the issue description don't give me this problem
@ahmedGaber93 You mean exact null
values right? because Onyx can still return an object that includes keys that are undefined
. This would also lead to report actions being filtered out by Boolean(value)
@mountiny @neil-marcellini
Details
Removes
null
values from OnyxCache (reads) and therefore removes unnecessary calls toremoveNestedNullValues
on read.This PR adds a
nullishStorageKeys
Set toOnyxCache
where a key can be added in order for the key to representnull
/"not set" in cache. This change let's us keep existing performance and loading state improvements that @janicduplessis implemented in https://github.com/Expensify/react-native-onyx/pull/401.This PR also adds some general code quality improvements around using the cache and handling reads and writes to Onyx.
Recently
IDBKeyvalProvider
hasn't been identical in it's behavior toSQLiteStorageProvider
. Therefore, i added some extra logic tosetItem
,multiSet
andmultiMerge
, to check for null values and handle those in the storage layer.Related Issues
https://github.com/Expensify/react-native-onyx/issues/553 https://github.com/Expensify/App/issues/42654
Automated Tests
Manual Tests
Author Checklist
### Related Issues
section aboveTests
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop