-
Notifications
You must be signed in to change notification settings - Fork 0
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
Relays and mailboxes #3
Conversation
src/relay/relay.ts
Outdated
return String(response.data); | ||
} catch (e) { | ||
console.log(e.response); | ||
throw new Error('[Relay] Bad Response'); |
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.
error will be same for every erro that axios returns No Connection, 5xx, 4xx. I propose to handle only this line return String(response.data);
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'm now throwing HTTP status there, so the client app can try
a network call and catch 401
, 404
, 500
or whatever. Is it fine, or do you have better suggestions?
https://github.com/vault12/glow.ts/blob/relays-and-mailboxes/src/relay/relay.spec.ts#L28-L40
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 propose to throw new HttpErrorResponse(axiosError.response.status) we already handle it in vault12 https://github.com/vault12/vault12/blob/4fa0dc5d0e629c79d9ac6607c4d638258dbd443e/src/app/core/communication/network-retry-strategy/network-retry-strategy.ts#L119-L125 or just remove try catch
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.
OK, makes sense. I removed try-catch, let the library user decide what to do with errors
No description provided.