Bump onyx to 2.0.48 (after revert) #42772

chrispader · 2024-05-29T15:31:06Z

Details

Re-applies the Onyx bump from #42057 after applying the fix in react-native-onyx@2.0.42 from Expensify/react-native-onyx#554.

This also applies more Onyx updates up to version 2.0.48.

This PR contains all the previous changes from the original Onyx bump. In order to more easily work on and review the changes required by the new null-handling implemented in Expensify/react-native-onyx#554, i'll add all new changes in margelo#20

~~Before we merge this bump PR, we're going to merge margelo#20 into this PR first~~

List of all the Onyx changes/PRs that were introduced with in this bump

(MAJOR) - Might affect usage of Onyx in E/App and can cause unexpected behavior or (positive) changes in performance
(minor) - Either internal changes or changes that will not affect how we use and persist data with Onyx

(MAJOR) Fixed inconsistences between single key and multi key operations:
This PR removed some inconsistencies about how we handle null values and wether we remove those from cache and storage or not. These changes are extended even further in later, so they fit in the greater picture of this Onyx bump.
(minor) DEV - Throws warning when starting desktop:
This PR just adds a proper type check to the DevTools module. Nothing production-critical.
(minor) Add some basic runtime type validation in Onyx :
This PR added runtime validation to setter operations which checks if a value that is being set/merged is compatible with the existing value in Onyx. E.g. you cannot set/merge an object for a key which has an existing value of type Array
(minor) Fix: collection subscriber updated when nothing changed:
This PR prevents Onyx from updating subscribers (withOnyx/Onyx.connect) for keys where the value has not changed.
(minor) useOnyx type improvements:
This PR just contains changes to the TS types for useOnyx
(MAJOR) Perf: use one loop instead of three for getCachedCollection:
This PR improved performance of many Onyx getter operations by speeding up the getCachedCollection util function.
(minor) Fix withOnyx initWithStoredValues doesn't work:
This PR fixes an issue where withOnyx would not respect the mapping option initiWithSoredValues and therefore would still initialize the property with the stored value
(minor) [TS migration] Migrate tests in react-native-onyx:
This PR just migrates the test suite in Onyx to TypeScript
(minor) [TS] Make useOnyx result type NOT readonly:
This PR just fixes a typing issue introduced by removing null values from storage/cache and because we now only return undefined to the user.
(minor) [TS migration] Migrate withOnyx.js to TypeScript:
This PR just migrates withOnyx to TypeScript
(MAJOR) Remove null values from cache to improve Onyx read speed:
This PR is very much the most critical and important PR in the whole bump. It fixes the (performance) regressions introduced in the previous Onyx bump PR to 2.0.41 and improves performance in general. It aims to remove null values from cache (and storage) completely and change Onyx so that it never returns null to the user. It introduces a new mechanism to OnyxCache which lets us define keys, that have been fetched from storage before, without setting the value to null in cache. This allows us, to easily return values from cache in Onyx getter methods, without needing to refactor the values on read. This improves read speed by a lot.
(minor) [TS] Onyx input method types allow (deep) nullish values in input methods and disallow them in outputs:
This PR just changes some TS types to not allow null from being returned from Onyx while specifically allowing null values in setter functions, to indicate removing a key from storage.
(minor) README update to promote using useOnyx over withOnyx:
We recently deprecated withOnyx and therefore needed to update the README
(minor) Update node version to match latest
Updates node to the version in E/App
(MAJOR) Fix: issues in Onyx bump due to removal of null values in cache:
This is more or less an extension and enhancement of (MAJOR) Remove null values from cache to improve Onyx read speed, which fixes some issues that were discovered during testing the Onyx bump PR. It fixes some edge cases where values in cache and storage would still be set to null.
(minor) More minor fixes for the Onyx bump in E/App:
This PR fixes an issue in useOnyx where the hook would never resolve the loading state. This stemmed from not returning null from cache anymore, when a value has been read. This PR also updates some tests to reflect recent changes.

PRs with changes in E/App necessary to reflect new Onyx changes

We split this PR into 2 separate PRs to allow easier reviews and to not introduce any new regressions for logic, that already worked. These are the two PRs that were merged together as one in this PR:

Bump onyx to 2.0.48 (after revert) (this PR):
This PR originally just included the changes from the previous Onyx bump PR to 2.0.41 and was also tested and reviews as such. We checked if all of the previously discovered issues were still fixed and if any new issues arose after the revert. Later on we merged the second PR (2.) into this one and merged it together.
Onyx Bump 2.0.48: null -> undefined changes:
This PR included all new needed changes because of the removal of null values from cache/storage in (MAJOR) Remove null values from cache to improve Onyx read speed. The PR mostly includes changes of values and types in E/App from null to undefined and fixes all issues that originated from these changes. Once the PR was reviewed and tested thoroughly, it was merged in THIS PR.

Fixed Issues

$ #41531
$ #41895

Tests

Verify that no errors appear in the JS console

Address page country selection

Go to Settings -> Profile -> Address
Either set or change the address to some address in the US.
Save the address
Edit the address again
Start typing a UK address (e.g. "226 Baker Street").
Select the UK address from the list
Make sure the country changes to UK and validation of the zip code works.

Offline tests

QA Steps

Verify that no errors appear in the JS console

We'll need to do a full QA-testing run on this PR, as it contains lots of changes that might cause regressions.

PR Author Checklist

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

…2057" This reverts commit c956751, reversing changes made to 1183a92.

melvin-bot · 2024-05-29T15:31:34Z

@techievivek Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

…ispader/onyx-2-0-43-null-undefined-changes

mountiny

Major effort, changes are looking good to me, mainly updating the types and making sure the app does not expect null from onyx.

@ikevin127 @s77rt could you please complete the checklist on this one?

s77rt

I noticed there are many cases where we fallback to null in cases where this is not necessary

src/components/AddressForm.tsx

src/components/AttachmentModal.tsx

src/components/MoneyRequestConfirmationList.tsx

src/libs/Network/NetworkStore.ts

src/libs/OptionsListUtils.ts

src/pages/home/report/SystemChatReportFooterMessage.tsx

src/libs/actions/User.ts

src/libs/actions/Session/index.ts

s77rt · 2024-06-12T17:55:27Z

Reports on Search page are laggy, opening and closing reports is not very responsive (takes some time)

Screen.Recording.2024-06-12.at.6.48.51.PM.mov

Screen.Recording.2024-06-12.at.7.35.45.PM.mov

ikevin127

Thanks for addressing the issues and requirements from #42772 (comment).
All requirements were met -> Approved.

I found a new issue while double-checking the other issues but this is present on latest main branch so not related to this PR:

Note

Not present on staging nor last PR's build from #42772 (comment). Only present on latest main (local dev) and this PR because of syncing with main.

All popups have the first option highlighted when opened

Not caused by this PR as it's also reproducible on latest main (local dev).

Note: Potentially caused by PR Add focus trap #39520 (awaiting confirmation) which was merged but not yet deployed to staging / prod.

Video

web.mov

chrispader · 2024-06-12T21:46:35Z

Reports on Search page are laggy, opening and closing reports is not very responsive (takes some time)

i can reproduce this partly. It seems, we have more renders than before when we mount the screen (in the right pane) while navigating.

I'm not sure if this is actually caused by this PR but if we shouldn't just optimize ReportScreen, because i feel the loading states in this screen aren't perfect.

I think this is also partly related to the changes in Onyx we needed for this to load at all:

Screen.Recording.2024-06-12.at.23.42.34.mov

chrispader · 2024-06-12T21:48:49Z

Sometimes it's pretty smooth all the time, so can we live with this for now or should we try to improve this in this Onyx bump PR?

(also i'm not sure if these blank chats are intended?)

Screen.Recording.2024-06-12.at.23.49.02.mov

ikevin127 · 2024-06-12T22:07:57Z

Note

Be mindful of Add focus trap #39520 which was merged ~5h ago and might cause bugs which might be reported as deploy blockers of this PR post-merge, since both are big PRs and touch a lot of code.

The merged PR already caused a bug which I reported in #42772 (review) and verified in #39520 (comment) (awaiting confirmation from author).

mountiny

Going to ship this now, lets be on a look out for the regressions

OSBotify · 2024-06-12T22:59:55Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

kavimuru · 2024-06-13T12:45:01Z

We'll need to do a full QA-testing run on this PR, as it contains lots of changes that might cause regressions.

@mountiny should we rerun again with full regression. We did this one last week #42772 (comment)

mountiny · 2024-06-13T13:00:23Z

@kavimuru nono, the C+ confirmed all the regressions been cleared

kavimuru · 2024-06-13T14:09:13Z

@mountiny Got it, then what steps should our QA team have to run for this PR?

mountiny · 2024-06-14T13:11:04Z

Nothing specific, it touches lots of places so this should be covered by the generic regression testing

lanitochka17 · 2024-06-14T13:19:26Z

IOU - "Content-Security-Policy directive" console error when tracking and requesting expense

Version Number: 1.4.80-16
Reproducible in staging?: N
Reproducible in production?: N
Issue reported by: Applause-Internal team

Action Performed:

Navigate to https://42772.pr-testing.expensify.com/
Log in with a new expensifail account
Open Chrome console
Create a new workspace
Tap on FAB - Track expense
Tap on FAB - Submit expense

Expected Result:

There shouldn't be any console errors

Actual Result:

"Content-Security-Policy directive name 'https://sdk.onfido.com'/
contains one or more invalid characters." console error appears when tracking expense. Also repro by opening Submit expense.

15:06:20.799 The Content-Security-Policy directive name 'https://sdk.onfido.com'/
contains one or more invalid characters. Only ASCII alphanumeric characters or dashes '-' are allowed in directive names.
15:06:20.800 The Content-Security-Policy directive name 'https://sdk.onfido.com'/
contains one or more invalid characters. Only ASCII alphanumeric characters or dashes '-' are allowed in directive names.
15:06:20.800 /track-training:1 The Content-Security-Policy directive name 'https://sdk.onfido.com'/
contains one or more invalid characters. Only ASCII alphanumeric characters or dashes '-' are allowed in directive names.
15:06:20.800 /track-training:1 The Content-Security-Policy directive name 'https://sdk.onfido.com'/
contains one or more invalid characters. Only ASCII alphanumeric characters or dashes '-' are allowed in directive names.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

iOS: Native
MacOS: Chrome / Safari

Screenshots/Videos

Bug6506212_1717852556645.bandicam_2024-06-08_15-06-17-093.mp4

mountiny · 2024-06-14T14:24:14Z

That should be related to the adhoc build

OSBotify · 2024-06-14T23:03:07Z

🚀 Deployed to production by https://github.com/mountiny in version: 1.4.83-3 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

rushatgabhane · 2024-10-09T19:19:03Z

src/libs/actions/PriorityMode.ts

 Onyx.connect({
    key: ONYXKEYS.NVP_TRY_FOCUS_MODE,
    callback: (val) => {
-        hasTriedFocusMode = val;
+        hasTriedFocusMode = val ?? null;


This caused a bug. #48917

Focus mode notification kept showing on sign in

chrispader added 2 commits May 29, 2024 12:56

Revert "Merge pull request Expensify#42725 from Expensify/vit-revert4…

31e6e87

…2057" This reverts commit c956751, reversing changes made to 1183a92.

update package-lock.json

760f8b6

chrispader requested a review from a team as a code owner May 29, 2024 15:31

melvin-bot bot requested review from techievivek and removed request for a team May 29, 2024 15:31

chrispader mentioned this pull request May 29, 2024

Remove null values from cache to improve Onyx read speed Expensify/react-native-onyx#554

Merged

42 tasks

mountiny requested review from mountiny and ikevin127 May 30, 2024 11:11

mountiny assigned chrispader May 30, 2024

Merge branch 'main' into @chrispader/after-revert-bump-onyx-to-2-0-42

6322c8f

chrispader changed the title ~~[HOLD on react-native-onyx#554] Bump onyx to 2.0.42 (after revert)~~ [HOLD on react-native-onyx#554] Bump onyx to 2.0.43 (after revert) May 30, 2024

chrispader force-pushed the @chrispader/after-revert-bump-onyx-to-2-0-42 branch from c20839a to 6322c8f Compare May 31, 2024 07:19

chrispader added 2 commits May 31, 2024 09:20

Merge branch 'main' into @chrispader/after-revert-bump-onyx-to-2-0-42

317b5ab

update to Onyx 2.0.43

2867f5e

chrispader mentioned this pull request May 31, 2024

[WIP] Onyx Bump 2.0.43: null -> undefined changes #42898

Closed

50 tasks

chrispader changed the title ~~[HOLD on react-native-onyx#554] Bump onyx to 2.0.43 (after revert)~~ Bump onyx to 2.0.43 (after revert) May 31, 2024

fix: report related null chnages

cb0711e

chrispader mentioned this pull request May 31, 2024

Onyx Bump 2.0.48: null -> undefined changes margelo/expensify-app-fork#20

Merged

50 tasks

chrispader added 11 commits May 31, 2024 11:25

migrate more tests

1fde464

migrate more components

903b841

migrate more components

74e120c

migrate more files

e35b748

migrate more files

cfb7b2d

fix: migrate more files

0778946

fix: tests

8c5ff84

Merge branch 'main' into @chrispader/onyx-2-0-43-null-undefined-changes

d0d896b

update to onyx 2.0.44

2bd8bdf

Merge branch 'main' into @chrispader/after-revert-bump-onyx-to-2-0-42

f51e241

Merge branch '@chrispader/after-revert-bump-onyx-to-2-0-42' into @chr…

7072973

…ispader/onyx-2-0-43-null-undefined-changes

mountiny previously approved these changes Jun 12, 2024

View reviewed changes

Merge branch 'main' into @chrispader/after-revert-bump-onyx-to-2-0-42

dfb0b2c

chrispader dismissed mountiny’s stale review via dfb0b2c June 12, 2024 16:49

s77rt reviewed Jun 12, 2024

View reviewed changes

ikevin127 approved these changes Jun 12, 2024

View reviewed changes

melvin-bot bot requested a review from mountiny June 12, 2024 21:00

chrispader added 3 commits June 12, 2024 23:24

fix: don't fallback to null

bfcf8b3

fix: return authToken as is from NetworkStore

03f243d

fix: remaining issues

e115cdc

ikevin127 mentioned this pull request Jun 12, 2024

Add focus trap #39520

Merged

50 tasks

mountiny approved these changes Jun 12, 2024

View reviewed changes

mountiny merged commit a451de4 into Expensify:main Jun 12, 2024
18 checks passed

github-actions bot mentioned this pull request Jun 13, 2024

Deploy Checklist: New Expensify 2024-06-13 #43655

Closed

64 tasks

kbecciv mentioned this pull request Jun 13, 2024

[$250] Pay someone - Console error shows up when changing payment #43691

Closed

6 tasks

ikevin127 mentioned this pull request Jun 15, 2024

Profile - Address - Zip code verification does not work correctly #41531

Closed

6 tasks

OlimpiaZurek mentioned this pull request Jun 19, 2024

[NOQA] Fix Reassure test after updating Onyx #44004

Merged

50 tasks

roryabraham mentioned this pull request Jun 20, 2024

[Ready for payment][$250] Add some basic runtime type validation in Onyx #39852

Closed

rushatgabhane reviewed Oct 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump onyx to 2.0.48 (after revert) #42772

Bump onyx to 2.0.48 (after revert) #42772

chrispader commented May 29, 2024 •

edited

Loading

melvin-bot bot commented May 29, 2024

mountiny left a comment

s77rt left a comment

s77rt commented Jun 12, 2024 •

edited

Loading

ikevin127 left a comment •

edited

Loading

chrispader commented Jun 12, 2024

chrispader commented Jun 12, 2024 •

edited

Loading

ikevin127 commented Jun 12, 2024

mountiny left a comment

OSBotify commented Jun 12, 2024

kavimuru commented Jun 13, 2024 •

edited

Loading

mountiny commented Jun 13, 2024

kavimuru commented Jun 13, 2024

mountiny commented Jun 14, 2024

lanitochka17 commented Jun 14, 2024

mountiny commented Jun 14, 2024

OSBotify commented Jun 14, 2024

rushatgabhane Oct 9, 2024

Bump onyx to 2.0.48 (after revert) #42772

Bump onyx to 2.0.48 (after revert) #42772

Conversation

chrispader commented May 29, 2024 • edited Loading

Details

List of all the Onyx changes/PRs that were introduced with in this bump

PRs with changes in E/App necessary to reflect new Onyx changes

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented May 29, 2024

mountiny left a comment

Choose a reason for hiding this comment

s77rt left a comment

Choose a reason for hiding this comment

s77rt commented Jun 12, 2024 • edited Loading

ikevin127 left a comment • edited Loading

Choose a reason for hiding this comment

chrispader commented Jun 12, 2024

chrispader commented Jun 12, 2024 • edited Loading

ikevin127 commented Jun 12, 2024

Note

mountiny left a comment

Choose a reason for hiding this comment

OSBotify commented Jun 12, 2024

kavimuru commented Jun 13, 2024 • edited Loading

mountiny commented Jun 13, 2024

kavimuru commented Jun 13, 2024

mountiny commented Jun 14, 2024

lanitochka17 commented Jun 14, 2024

IOU - "Content-Security-Policy directive" console error when tracking and requesting expense

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

mountiny commented Jun 14, 2024

OSBotify commented Jun 14, 2024

rushatgabhane Oct 9, 2024

Choose a reason for hiding this comment

chrispader commented May 29, 2024 •

edited

Loading

s77rt commented Jun 12, 2024 •

edited

Loading

ikevin127 left a comment •

edited

Loading

chrispader commented Jun 12, 2024 •

edited

Loading

kavimuru commented Jun 13, 2024 •

edited

Loading