chore(core): fallback for language query #4354
Conversation
UncleSamtoshi
commented
Apr 24, 2024
•
UncleSamtoshi
commented
- use empty language when notifictions service returns an error
- add timeout to notification settings grpc call
UncleSamtoshi
force-pushed
the
notification-error-fallback
branch
from
fafac39 to
a298a0a
Compare
| @@ -39,6 +39,8 @@ export const checkedToEmailAddress = ( | |||
| return emailAddress as EmailAddress | |||
| } | |||
|
|
|||
| export const DefaultLanguage = "" as UserLanguageOrEmpty | |||
There was a problem hiding this comment.
With the default being set the type UserLanguageOrEmpty becomes irrelevant. Perhaps you could find something semantically more meaningful. Or if it's too complicated to untangle we can leave it for another PR.
Looks good otherwise.
There was a problem hiding this comment.
Not sure I fully understand. The DefaultLanguage is used as a fallback in the case of the error, but even in the case there is not an error the notifications service could still return an empty string implying that the mobile application should use the OS system default. It's definitely a little bit of a mess that is a relic of how the mobile application was handling the language.
There was a problem hiding this comment.
I think it may make sense to save that refactoring for a separate pr.
| @@ -65,6 +64,9 @@ export const NotificationsService = (): INotificationsService => { | |||
| const response = await notificationsGrpc.getNotificationSettings( | |||
| request, | |||
| notificationsGrpc.notificationsMetadata, | |||
| { | |||
There was a problem hiding this comment.
🤔 shouldn't we use something similar to https://github.com/GaloyMoney/galoy/blob/main/core/api/src/services/lnd/index.ts#L801 ?
There was a problem hiding this comment.
That would work too, but I don't see a problem with relying on grpc.
