8000
refactor: join options logic from http and fetchr client modules #348
Conversation
This file contains hidden or 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
4f99a62 to
1e54dec
Compare
redonkulus
reviewed
Jan 11, 2022
| return response.text().then(function (responseBody) { | ||
| return parseResponse(responseBody); | ||
| return response.json().catch(function () { | ||
| return null; |
There was a problem hiding this comment.
Yup :( This is the fetchr behavior since a looong time ago. For me, we could throw an error in this case as well, but I'm not sure yet if we would be fixing or introducing a bug.
I need to add some functional tests to properly see how fetchr will behave with a 200 response and empty body. I think unless fetchr is not properly set in the server, there is no way to get a empty response...
redonkulus
approved these changes
Jan 11, 2022
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.
This is the last big refactoring one. Here, I'm merging the code related to options normalization from the
http.clientandfetchr.cleintmodules. As you can see, when you put both together, we have a bunch of code (that I'll probably simplify in another PR).By centralizing the options we basically create only one interface that is passed down to all functions (it will be helpful when migrating to ts).
But more importantly,
httpRequestdoes not compute the options unnecessarily on every retry. This makes the plate clean to fix the corner cases that I mentioned in the other PR:There will be also another PR improving the
FetchrErrorclass:Error;optionsobject;name,messageandreasonfields to make it easier for users to report/log errors;Since all this changes should be backward compatible, I would like to do it before migrating to ts/change the API.
I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.