refactor(http): remove callbacks, simplify options normalization and export only one function #347
Conversation
|
@pablopalacios is this part of the 1.0 breaking changes / api refactor work? |
|
Looking at the code, I wonder if we should just remove all the .then() calls and switch to async / await everywhere. Especially if we are removing callbacks. Its much easier to maintain and read. Then we can just document that this library only works with latest browsers and node versions. |
| @@ -128,11 +128,11 @@ Request.prototype._captureMetaAndStats = function (err, result) { | |||
| Request.prototype.end = function (callback) { | |||
There was a problem hiding this comment.
This callback is still required?
| var baseUrl = config.uri; | ||
| if (!baseUrl) { | ||
| baseUrl = config.cors | ||
| ? request.options.corsPath | ||
| : request.options.xhrPath; | ||
| } | ||
|
|
||
| if (request.operation === OP_READ) { | ||
| if (request.operation === OP_READ && !config.post_for_read) { |
There was a problem hiding this comment.
post for read is weird, I know this is old, what is the use case for this again?
There was a problem hiding this comment.
To be really honest, I don't know. I see in my code base that we used to use create instead of read just to make sure we would have a POST. I think the reason for that was related to how our bot protection used to work. In that case, calling read with post_for_read would be semantically better.
There was a problem hiding this comment.
By the way, I checked that the support exists since the very first implementation. Don't you have any usage of it in your code base?
There was a problem hiding this comment.
I was always under the impression that POST = create and GET = read, I'm not really sure of all the different usage of fetchr internally, we have many different apps using it.
We now return a promise from http module methods. fetcher.client is now the only responsible for handling the callback flow.
pablopalacios
force-pushed
the
http-refactor
branch
from
d1f0333
to
5628650
Compare
Actually no (but it definitely will help). I've found some corner cases that are not being covered regarding aborts, timeouts and retries. But in order to fix it, the http module must be simplified first. So before any breaking change, I would like to tidy up the internals and have http working 100%.
I'm not so sure if we can fully migrate to async/await. Since we have abort as part of the API, we must return something to the user before getting the response. But I think after tidying up the code, it will get easier to understand (even though I think doing a fetch-abort-retry-timeout will always end up in a not so straightforward code). |
Summary of changes:
getandpostwith only one method:httpRequesthttpRequestalways returns a promise (fetcher is now the only responsible for handling the callback flow)Again, I recommend you to review commit by commit ;)
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.