Fix several bugs on HttpNativeImpl
#626
Conversation
dahlia
changed the title
dahlia
force-pushed
the
trim-mime-types-parameters
branch
from
8b61ea4
to
dbe520a
Compare
dahlia
changed the title
HttpNativeImpl
src/http/http-axios-impl.ts
Outdated
| headers['Content-Type'] ?? 'application/json', | ||
| (headers['Content-Type'] ?? 'application/json') | ||
| .get('Content-Type') | ||
| .replace(/\s*;.*$/, ""), |
There was a problem hiding this comment.
I'd like to avoid repeating this code in both axios-impl and native-impl because I'm going to keep maintaining both implementations until fetch is well supported on Node.js.
How about adding something like getContentType(headers): string to BaseHttp class so that they can call the shared inherited method?
There was a problem hiding this comment.
That sounds way better. I'm going to extract duplicated code into a common method.
src/http/http-native-impl.ts
Outdated
| @@ -37,11 +47,14 @@ export class HttpNativeImpl extends BaseHttp implements Http { | |||
| body: body as string, | |||
| }); | |||
| const text = await response.text(); | |||
| const contentType = response.headers | |||
There was a problem hiding this comment.
This block-scoped variable contentType has the same name and role as the variable in line 30.
dahlia
force-pushed
the
trim-mime-types-parameters
branch
from
dbe520a
to
f277388
Compare
|
@neet I did my best to address everything you reviewed. Could you take a look again? |
|
🎉 This PR is included in version 4.3.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Trim content type parameters from HTTP responses
Previously,
request()method fromHttpAxiosImpl&HttpNativeImplsometimes threw an error like below:It happened when a response from server contains
Content-Typeheader with some parameters, e.g.,application/json; charset=utf-8.It might be better to make
deserialize()method fromSerializerNodejsImpl&SerializerNativeImplto trim parameters from their input, but I trim parameters from response content types instead. If you leave a comment on this I'm going to address that.On
HttpNativeImpl.request<T>()'s return typeAlso there's one more fix on
HttpNativeImpl.request<T>()method. It should returnResponse<T>, but actually returnedTinstead. I addressed this bug as well.Let
HttpNativeImplset an appropriate boundary formultipart/form-dataThe standard
fetch()API automatically configures an arbitrary boundary formultipart/form-datawhen its body isFormData. However, the boundary can be omitted ifContent-Typeheader is manually set. I madeHttpNativeImplto leaveContent-Typeheader unset when it'smultipart/form-dataso that the underlyingfetch()can determine an appropriate boundary for the form data.