Skip to content
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

Handling errors #15

Closed
remko opened this issue May 14, 2021 · 7 comments · Fixed by #17
Closed

Handling errors #15

remko opened this issue May 14, 2021 · 7 comments · Fixed by #17

Comments

@remko
Copy link
Contributor

remko commented May 14, 2021

The fetch function seems to ignore the .ok status of the fetch response. This means that if there is a gRPC error, data is returned of the error type instead of the declared response type.

Shouldn’t the fetch function throw in case of http errors (I.e. if .ok is false), so the client can catch errors? (And so there is no typing inconsistency)

@lyonlai
Copy link
Collaborator

lyonlai commented May 18, 2021

gRPC error returned from grpc-gateway is still http 200 but with error object as JSON returned iirc.

@atreya2011
Copy link
Contributor

@remko You could just check the error object instead.

@remko
Copy link
Contributor Author

remko commented May 18, 2021

@lyonlai No,grpc-gateway maps gRPC codes to HTTP

< HTTP/1.1 401 Unauthorized
< Content-Type: application/json; charset=utf-8
< Date: Tue, 18 May 2021 05:46:57 GMT
< Content-Length: 40
< 
{"status":401,"message":"Unauthorized"}

@remko
Copy link
Contributor Author

remko commented May 18, 2021

@atreya2011 There's no unambiguous way to check for an error object at that level. I would have to check for the presence of status and message properties, which could exist on a regular object too (e.g. a chat service presence message would have those fields)

Moreover, I would need to manually do this check for every call, or at least manually wrap every call, which is tedious.

@lyonlai
Copy link
Collaborator

lyonlai commented May 18, 2021

hmm. I see, that make sense. @remko do you have something already and you wanted to contribute?

@remko
Copy link
Contributor Author

remko commented May 18, 2021

@lyonlai I don't, but I should be able to easily do, just wanted to check up front. I will do the changes and verify it with my use case, and send a PR.

@lyonlai
Copy link
Collaborator

lyonlai commented May 18, 2021

awesome, expecting it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants