-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bump onyx to 2.0.48 (after revert) #42772
Bump onyx to 2.0.48 (after revert) #42772
Conversation
@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] |
c20839a
to
6322c8f
Compare
…ispader/onyx-2-0-43-null-undefined-changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed there are many cases where we fallback to null in cases where this is not necessary
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.movScreen.Recording.2024-06-12.at.7.35.45.PM.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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 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 |
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 |
Note
The merged PR already caused a bug which I reported in #42772 (review) and verified in #39520 (comment) (awaiting confirmation from author). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to ship this now, lets be on a look out for the regressions
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
@mountiny should we rerun again with full regression. We did this one last week #42772 (comment) |
@kavimuru nono, the C+ confirmed all the regressions been cleared |
@mountiny Got it, then what steps should our QA team have to run for this PR? |
Nothing specific, it touches lots of places so this should be covered by the generic regression testing |
IOU - "Content-Security-Policy directive" console error when tracking and requesting expenseVersion Number: 1.4.80-16 Action Performed:
Expected Result:There shouldn't be any console errors Actual Result:"Content-Security-Policy directive name 'https://sdk.onfido.com'/ 15:06:20.799 The Content-Security-Policy directive name 'https://sdk.onfido.com'/ Workaround:Unknown Platforms:Which of our officially supported platforms is this issue occurring on?
Screenshots/VideosBug6506212_1717852556645.bandicam_2024-06-08_15-06-17-093.mp4 |
That should be related to the adhoc build |
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.83-3 🚀
|
@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 firstList 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
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.
This PR just adds a proper type check to the
DevTools
module. Nothing production-critical.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 typeArray
This PR prevents Onyx from updating subscribers (
withOnyx
/Onyx.connect
) for keys where the value has not changed.This PR just contains changes to the TS types for
useOnyx
This PR improved performance of many Onyx getter operations by speeding up the
getCachedCollection
util function.This PR fixes an issue where
withOnyx
would not respect the mapping optioninitiWithSoredValues
and therefore would still initialize the property with the stored valueThis PR just migrates the test suite in Onyx to TypeScript
This PR just fixes a typing issue introduced by removing
null
values from storage/cache and because we now only returnundefined
to the user.This PR just migrates
withOnyx
to TypeScriptThis 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 returnsnull
to the user. It introduces a new mechanism toOnyxCache
which lets us define keys, that have been fetched from storage before, without setting the value tonull
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.This PR just changes some TS types to not allow
null
from being returned from Onyx while specifically allowingnull
values in setter functions, to indicate removing a key from storage.useOnyx
overwithOnyx
:We recently deprecated
withOnyx
and therefore needed to update the READMEUpdates node to the version in E/App
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
.This PR fixes an issue in
useOnyx
where the hook would never resolve the loading state. This stemmed from not returningnull
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:
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.
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 fromnull
toundefined
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
Address page country selection
Settings -> Profile -> Address
Offline tests
QA Steps
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
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.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