Bump onyx to 2.0.48 (after revert)

[chrispader]

@mountiny

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

1. (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.
2. (minor) DEV - Throws warning when starting desktop:
   This PR just adds a proper type check to the DevTools module. Nothing production-critical.
3. (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
4. (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.
5. (minor) useOnyx type improvements:
   This PR just contains changes to the TS types for useOnyx
6. (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.
7. (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
8. (minor) [TS migration] Migrate tests in react-native-onyx:
   This PR just migrates the test suite in Onyx to TypeScript
9. (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.
10. (minor) [TS migration] Migrate withOnyx.js to TypeScript:
    This PR just migrates withOnyx to TypeScript
11. (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.
12. (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.
13. (minor) README update to promote using useOnyx over withOnyx:
    We recently deprecated withOnyx and therefore needed to update the README
14. (minor) Update node version to match latest
    Updates node to the version in E/App
15. (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.
16. (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:

1. 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.
2. 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

1. Go to Settings -> Profile -> Address
2. Either set or change the address to some address in the US.
3. Save the address
4. Edit the address again
5. Start typing a UK address (e.g. "226 Baker Street").
6. Select the UK address from the list
7. 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

• I linked the correct issue in the ### Fixed Issues section above
• I wrote clear testing steps that cover the changes made in this PR
  • I added steps for local testing in the Tests section
  • I added steps for the expected offline behavior in the Offline steps section
  • I added steps for Staging and/or Production testing in the QA steps section
  • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• I included screenshots or videos for tests on all platforms
• I ran the tests on all platforms & verified they passed on:
  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop
• I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• I followed proper code patterns (see Reviewing the code)
  • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
  • I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
  • I verified that comments were added to code that is not self explanatory
  • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
  • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • I verified the JSDocs style guidelines (in STYLE.md) were followed
• If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• I followed the guidelines as stated in the Review Guidelines
• I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• I verified that if a function's arguments changed that all usages have also been updated correctly
• If any new file was added I verified that:
  • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• If a new CSS style is added I verified that:
  • A similar style doesn't already exist
  • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
• If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
• If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
• If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
  • I verified that all the inputs inside a form are aligned with each other.
  • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
• If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

[melvin-bot]

@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]

[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

[s77rt]

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]

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]

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]

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]

✋ 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]

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]

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

[kavimuru]

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

[mountiny]

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

[lanitochka17]

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:

1. Navigate to https://42772.pr-testing.expensify.com/
2. Log in with a new expensifail account
3. Open Chrome console
4. Create a new workspace
5. Tap on FAB - Track expense
6. 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]

That should be related to the adhoc build

[OSBotify]

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

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