Content-type handling in DELETE and PUT methods #77
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MattCCC
reviewed
Oct 12, 2024
MattCCC
reviewed
Oct 14, 2024
| body?: any, | ||
| ) => { | ||
| // For PUT and DELETE methods, do not set Content-Type if no body is provided. | ||
| if (['PUT', 'DELETE'].includes(method) && !body) { |
There was a problem hiding this comment.
Small performance improvement: we could check !body before calling the includes(). Might be faster for all requests that have body specified.
There was a problem hiding this comment.
Suggested change
| if (['PUT', 'DELETE'].includes(method) && !body) { | |
| if (!body && ['PUT', 'DELETE'].includes(method)) { |
I mean I was suggesting this one, not sure if you forgot 😄 I think we can merge and release after this change
There was a problem hiding this comment.
@saewoohan I think it was all good, I was about to merge it in fact 😄 Can you reopen please?
MattCCC
reviewed
Oct 14, 2024
MattCCC
reviewed
Oct 18, 2024
|
|
||
| // Automatically set Content-Type to 'application/json;charset=utf-8' if not already present. | ||
| if (headers instanceof Headers) { | ||
| if (!headers.has(CONTENT_TYPE)) { |
There was a problem hiding this comment.
[nit-pick] I'd suggest merging this nested if condition into the upper one and just use return;. This way we merge ifs, and remove else so it should save some bytes 😄
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Features:
Added Content-Type handling for DELETE and PUT methods.
First, checked if a body is provided. If no body is provided,
Then checked if Content-Type is explicitly set in requestConfig. If not explicitly set, remove the Content-Type header.
Additionally, I added some unit tests 😎
#75