-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Enable ESLint rule / DEV alert() to match with useOnyx canBeMissing
option
#59191
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
Enable ESLint rule / DEV alert() to match with useOnyx canBeMissing
option
#59191
Conversation
|
Can we bump Onyx straight to 2.0.96 so my PR gets there as well? |
|
canBeMissing
optioncanBeMissing
option
@parasharrajat 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] |
canBeMissing
optioncanBeMissing
option
This PR is waiting for Expensify/react-native-onyx#624 to be merged and available in a new release so we can use it here. Additionally, in order to keep the changes scoped I won't fix the ESLint errors (that are caused by the rule enabled in this very PR) |
|
canBeMissing
optioncanBeMissing
option
PR is ready to review @parasharrajat @iwiznia |
I believe the first time every key will always be empty. It needs to be loaded from the backend then this logic will alert it. So we are saying we will migrate all the |
|
@parasharrajat The plan is to migrate all But after thinking about it for a while, I just feel that we'll end up marking almost @iwiznia WDYT about this? |
canBeMissing
optioncanBeMissing
option
Looks like there was a newer Onyx Bump PR which was merged today, so this PR is focusing now only in enabling the ESLint rule and the DEV alert(). |
It's fine to set most of them to true as long as the code is ready to handle that, the problem right now is that a lot of places just assume some data will be loaded when it is not. |
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.
Looks good but you need to add the flag to the calls in Expensify.ts file, no?
We should update the Expensify.tsx file. |
const [screenShareRequest] = useOnyx(ONYXKEYS.SCREEN_SHARE_REQUEST); | ||
const [focusModeNotification] = useOnyx(ONYXKEYS.FOCUS_MODE_NOTIFICATION, {initWithStoredValues: false}); | ||
const [lastVisitedPath] = useOnyx(ONYXKEYS.LAST_VISITED_PATH); | ||
const [account] = useOnyx(ONYXKEYS.ACCOUNT, {canBeMissing: true}); |
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.
This key can be missing? Is it when you are logged out?
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.
const [account] = useOnyx(ONYXKEYS.ACCOUNT, {canBeMissing: true}); | ||
const [session] = useOnyx(ONYXKEYS.SESSION, {canBeMissing: false}); | ||
const [lastRoute] = useOnyx(ONYXKEYS.LAST_ROUTE, {canBeMissing: true}); | ||
const [userMetadata] = useOnyx(ONYXKEYS.USER_METADATA, {canBeMissing: true}); |
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.
Same here
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.
lastRoute
andscreenShareRequest
will alert during every refresh, even if logged inuserMetadata
will alert if logged out
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/iwiznia in version: 9.1.29-0 🚀
|
Explanation of Change
Fixed Issues
$ #58499
PROPOSAL:
Tests
useOnyx()
s of FreeTrial.tsx component to set thecanBeMissing
flag.Offline tests
Same as above.
QA Steps
Not possible for QA to test this.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)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
Screen.Recording.2025-03-24.at.17.06.34-compressed.mov
Android: mWeb Chrome
I'm having problems with my emulators when opening the Chrome app (they crash instantly), so I couldn't record videos for this platform.
iOS: Native
Screen.Recording.2025-03-24.at.17.38.16-compressed.mov
iOS: mWeb Safari
Screen.Recording.2025-03-24.at.17.39.48-compressed.mov
MacOS: Chrome / Safari
Screen.Recording.2025-03-24.at.17.42.18-compressed.mov
Screen.Recording.2025-03-24.at.17.43.10-compressed.mov
MacOS: Desktop
Screen.Recording.2025-03-24.at.17.46.40-compressed.mov