-
Notifications
You must be signed in to change notification settings - Fork 409
Using the new HttpClient in messaging module #301
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
Conversation
| error: response, | ||
| statusCode: 200, | ||
| }); | ||
| if (!response.isJson) { |
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 think it would be better to create a property or method to determine whether the response is valid, something like isValid() or valid.
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.
Renamed to contentJson and added some documentation. Didn't feel like naming this just json, since that seems to imply this returns a json object. wdyt?
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.
On seconds thoughts I renamed it back to isJson() and made it a method as you suggested. That feels like a better name for what this does.
src/utils/api-request.ts
Outdated
| ); | ||
| } | ||
|
|
||
| get isJson(): boolean { |
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.
If you need a method to check if a response is valid, I think it is better to give it another name. isValid(). or something like that. Also a isSomething() should be a method. If you need a property for this, call it something like valid.
Note JSCore style guide:
Avoid using the 'is' prefix for boolean properties unless it's unclear from context that the property is a boolean.
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.
It's not a matter of validity. It's about whether the response is json or not. We have endpoints (e.g. Metadata server), where the valid response is plain text. It also seems at least some of our backend endpoints return html error messages. So we need a way to easily differentiate between json and non-json responses, so the client code can handle them.
Using the new
AuthorizedHttpClientimplementation in the FCM code.This is a follow up of #280 and #291.
Also introducing the
isJsonfield to the HTTP response interface, to make it easier to handle JSON-formatted responses.