Conversation
|
@iwiznia About your comment:
I added support in Onyx Logger to pass extra data when logging stuff, and in E/App diff --git a/src/Expensify.tsx b/src/Expensify.tsx
index 13542380888..b01542872b1 100644
--- a/src/Expensify.tsx
+++ b/src/Expensify.tsx
@@ -4,6 +4,7 @@ import type {NativeEventSubscription} from 'react-native';
import {AppState, Linking, Platform} from 'react-native';
import type {OnyxEntry} from 'react-native-onyx';
import Onyx, {useOnyx} from 'react-native-onyx';
+import alert from './components/Alert';
import ConfirmModal from './components/ConfirmModal';
import DeeplinkWrapper from './components/DeeplinkWrapper';
import EmojiPicker from './components/EmojiPicker/EmojiPicker';
@@ -45,10 +46,14 @@ import type {Route} from './ROUTES';
import SplashScreenStateContext from './SplashScreenStateContext';
import type {ScreenShareRequest} from './types/onyx';
-Onyx.registerLogger(({level, message}) => {
+Onyx.registerLogger(({level, message, extraData}) => {
if (level === 'alert') {
Log.alert(message);
console.error(message);
+
+ if (extraData?.showAlert) {
+ alert(message);
+ }
} else if (level === 'hmmm') {
Log.hmmm(message);
} else {
But I still think it's too intrusive for devs and each alert stops the entire App's execution flow, please check out the video: Screen.Recording.2025-03-23.at.12.37.11.movI think we should only show the errors in the console. According to the checklist C+ should always check for new errors when reviewing PRs. WDYT? |
|
Let's try with alert at first and see if it is annoying? I understand the alerts are a bit annoying, but it only depends on how many you see normally. |
fabioh8010
changed the title
canBeMissing optioncanBeMissing option
iwiznia
left a comment
iwiznia
left a comment
There was a problem hiding this comment.
Can you add/update the readme/documentation to explain this new param please?
lib/useOnyx.ts
Outdated
| // If `canBeMissing` is set to `false` and the Onyx value of that key is not defined, | ||
| // we log an alert so it can be acknowledged by the consumer. | ||
| if (options?.canBeMissing === false && newStatus === 'loaded' && !isOnyxValueDefined) { | ||
| Logger.logAlert(`useOnyx returned no data for key '${key}' while canBeMissing was set to false.`, {showAlert: true}); |
There was a problem hiding this comment.
We need to do the alert() call if we are in dev, no?
There was a problem hiding this comment.
Also, can you move key to the params? I think otherwise this will create an issue for each key, for example one issue per reportID
There was a problem hiding this comment.
We need to do the alert() call if we are in dev, no?
We are going to handle this in E/App, with something like this in Expensify.tsx:
diff --git a/src/Expensify.tsx b/src/Expensify.tsx
index 13542380888..89f96ea5741 100644
--- a/src/Expensify.tsx
+++ b/src/Expensify.tsx
@@ -4,6 +4,7 @@ import type {NativeEventSubscription} from 'react-native';
import {AppState, Linking, Platform} from 'react-native';
import type {OnyxEntry} from 'react-native-onyx';
import Onyx, {useOnyx} from 'react-native-onyx';
+import alert from './components/Alert';
import ConfirmModal from './components/ConfirmModal';
import DeeplinkWrapper from './components/DeeplinkWrapper';
import EmojiPicker from './components/EmojiPicker/EmojiPicker';
@@ -45,10 +46,14 @@ import type {Route} from './ROUTES';
import SplashScreenStateContext from './SplashScreenStateContext';
import type {ScreenShareRequest} from './types/onyx';
-Onyx.registerLogger(({level, message}) => {
+Onyx.registerLogger(({level, message, extraData}) => {
if (level === 'alert') {
Log.alert(message);
console.error(message);
+
+ if (__DEV__ && extraData?.showAlert) {
+ alert(message);
+ }
} else if (level === 'hmmm') {
Log.hmmm(message);
} else {
There was a problem hiding this comment.
Also, can you move key to the params? I think otherwise this will create an issue for each key, for example one issue per reportID
@iwiznia Could you help me this? I'm not sure how I should pass this data. I saw some example here, so I guess we need to pass this way?
Logger.logAlert(`useOnyx returned no data for key '${key}' while canBeMissing was set to false.`, {key, showAlert: true});Also, I noticed that in E/App our Log.alert() has an additional parameters parameter that is used to pass this kind of data, right? In E/App we use Logger from expensify-common which has this signature. So I guess we must have same signature in our Onyx logger?
There was a problem hiding this comment.
We are going to handle this in E/App, with something like this in Expensify.tsx:
Ah ok, is that because __DEV__ is not accessible here or what?
There was a problem hiding this comment.
@iwiznia Could you help me this? I'm not sure how I should pass this data. I saw some example here, so I guess we need to pass this way?
Yep, but without the key in the message:
Logger.logAlert(`useOnyx returned no data for key with canBeMissing set to false.`, {key, showAlert: true});
Also, I noticed that in E/App our Log.alert() has an additional parameters parameter that is used to pass this kind of data, right? In E/App we use Logger from expensify-common which has this signature. So I guess we must have same signature in our Onyx logger?
So both signatures match already, no?
There was a problem hiding this comment.
Ah ok, is that because DEV is not accessible here or what?
It's because we don't have alert() here (it's a component file from E/App), and these kind of UI thing makes more sense to be handled by the consumer, not this type of library imo.
So both signatures match already, no?
Currently, no. I will look into this.
There was a problem hiding this comment.
@iwiznia I adjusted Onyx Logger functions to use same parameters signature we have in expensify-common, so code in Expensify.tsx will look like this:
diff --git a/src/Expensify.tsx b/src/Expensify.tsx
index 13542380888..3424147387d 100644
--- a/src/Expensify.tsx
+++ b/src/Expensify.tsx
@@ -4,6 +4,7 @@ import type {NativeEventSubscription} from 'react-native';
import {AppState, Linking, Platform} from 'react-native';
import type {OnyxEntry} from 'react-native-onyx';
import Onyx, {useOnyx} from 'react-native-onyx';
+import alert from './components/Alert';
import ConfirmModal from './components/ConfirmModal';
import DeeplinkWrapper from './components/DeeplinkWrapper';
import EmojiPicker from './components/EmojiPicker/EmojiPicker';
@@ -45,14 +46,18 @@ import type {Route} from './ROUTES';
import SplashScreenStateContext from './SplashScreenStateContext';
import type {ScreenShareRequest} from './types/onyx';
-Onyx.registerLogger(({level, message}) => {
+Onyx.registerLogger(({level, message, parameters}) => {
if (level === 'alert') {
- Log.alert(message);
+ Log.alert(message, parameters);
console.error(message);
+
+ if (typeof parameters === 'object' && !Array.isArray(parameters) && 'showAlert' in parameters) {
+ alert(message);
+ }
} else if (level === 'hmmm') {
- Log.hmmm(message);
+ Log.hmmm(message, parameters);
} else {
- Log.info(message);
+ Log.info(message, undefined, parameters);
}
});
amyevans
left a comment
amyevans
left a comment
There was a problem hiding this comment.
I don't have a lot to add here but I am a fan of the idea!
Co-authored-by: Amy Evans <amy@expensify.com>
Co-authored-by: Amy Evans <amy@expensify.com>
iwiznia
left a comment
iwiznia
left a comment
There was a problem hiding this comment.
Just a small nitpick change in the docs
Co-authored-by: Ionatan Wiznia <iwiznia@hotmail.com>
3 participants
Explanation of Change
Fixed Issues
$ Expensify/App#58499
PROPOSAL:
Tests
useOnyx()s of FreeTrial.tsx component to set thecanBeMissingflag.Offline tests
Same as above.
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Not possible for QA to test this.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.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