Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Response handling: callback / stream / promise #3

Open
analog-nico opened this issue May 2, 2016 · 5 comments
Open

Response handling: callback / stream / promise #3

analog-nico opened this issue May 2, 2016 · 5 comments

Comments

@analog-nico
Copy link
Member

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
    });
@simov
Copy link
Member

simov commented 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 bluebird in 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
Copy link
Member Author

analog-nico commented 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-promise based on request@next. Although you basically already did all the work I will create a configuration that produces the same api as the current version of request-promise. This way I can give you better feedback if everything falls into place like expected.

@analog-nico
Copy link
Member Author

I just refactored request-promise to use @request/api (basic type) and @request/client. Everything fell into place like expected. That means I will be able to create request-promise@next without additional breaking changes.

The request-promise tests that still fail can be considered as an effect of request@next still 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
Copy link
Member

simov commented 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
Copy link
Member Author

Perfect. 👍 Indeed, no rush there, I made the refactoring in a branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants