Repository navigation
Response handling: callback / stream / promise #3
Description
IMHO the interface would need polishing in regard to the response handling so that alternative implementations can pop up.
First off, the callback needs to be specified in detail. It can be modelled after Request's callback definition:
The callback argument gets 3 arguments:
1. An error when applicable (usually from http.ClientRequest object)
2. An http.IncomingMessage object
3. The third is the response body (String or Buffer, or JSON object if the json option is supplied)
However, when I read this I realise that especially the second parameter is highly implementation specific. In order to make implementations replaceable we would need to define a general response object I guess.
Handling responses as a stream or a promise would ideally be loaded as an optional dependency. In particular for the promise I would define:
request({ ... })
.then(function (response) {
// All responses get resolved with the general response object I mentioned for the callback
})
.catch(function (err) {
// Equivalent to the callback getting an error
});
Activity
simov commentedon May 2, 2016
I just tossed a few more details:
You can use the @request/api module in combination with any Promise implementation. The first example that I'm showing is using the built in Promise implementation, for cases when you don't want to include
bluebirdin your bundle.I'm currently working on a bunch of modules that are not pushed to GitHub yet, but that's the gist of it. I'm open for suggestions.
What I was showing in the first few examples was the most bare bone API that each HTTP client wrapper should implement - just a function that accepts an options object. As you can anything else can be built around that, and there is no need for the API implementation to be part of your module.
analog-nico commentedon May 3, 2016
Perfect @simov ! I reviewed all api sections and all looks good to me. It's so flexible. Kudos for your design!
I think at this point it is time for me to create an alpha version of
request-promisebased onrequest@next. Although you basically already did all the work I will create a configuration that produces the same api as the current version ofrequest-promise. This way I can give you better feedback if everything falls into place like expected.analog-nico commentedon May 6, 2016
I just refactored
request-promiseto use@request/api(basic type) and@request/client. Everything fell into place like expected. That means I will be able to createrequest-promise@nextwithout additional breaking changes.The
request-promisetests that still fail can be considered as an effect ofrequest@nextstill being alpha. They are related to implementation details and not to the new architecture. I won't report the issues right now because they most likely already pop up in request's own tests.simov commentedon May 6, 2016
That's great! Just keep in mind that the @request/core refactoring is not published yet, it just takes a bit more time. The only change for you would be that the module containing most of the request features enabled won't be called @request/client but something else probably. Other than that everything else should work as it is.
analog-nico commentedon May 6, 2016
Perfect. 👍 Indeed, no rush there, I made the refactoring in a branch.