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

Add support for promises over callbacks #796

Closed
auchenberg opened this issue Feb 3, 2014 · 12 comments
Closed

Add support for promises over callbacks #796

auchenberg opened this issue Feb 3, 2014 · 12 comments

Comments

@auchenberg
Copy link

Now that ES6 Promises are getting more stable, I would expect a lovely library like this eventually would support promises over callbacks.

I don't know how the roadmap is looking, but I'd love promises to my requests :)

@mikeal
Copy link
Member

mikeal commented Feb 3, 2014

This can be done trivially by a library and published. One already exists https://npmjs.org/package/request-promise although I'm not sure if it supports the same API.

The issue you're going to run in to though is that the return value from request is a stream. The callback option is for convenience but is often not even used in streaming mode. This is how request supports streaming and a callback option in the same API but you'd have to break streaming if you returned a promise instead.

@mikeal mikeal closed this as completed Feb 3, 2014
@auchenberg
Copy link
Author

Got it, makes sense. Didn't think more about the complications to the stream implementation.

@MajorBreakfast
Copy link

@mikeal What about request(...).end() -> returns promise

Personally I use var request = RSVP.denodeify(requre('request')) and don't use the streaming API. It would be great if promise support landed in request itself, though. Also maybe via var request = require('request').disableStreamingAPI() which returns a special request instance. Problem with wrapping by an external library is that any properties set on the wrapped instance don't get propagated to the original.

@mikeal
Copy link
Member

mikeal commented Feb 10, 2014

A promise library can wrap request quite easily to provide this API. There is no syntactic advantage to adding this to request itself, it's actually more readable when wrapped normally by the promise library.

@MajorBreakfast
Copy link

Thx for your answer :)
One more thing: Chaining APIs work with promises if the promise is only created through the call of .then(), .catch(), .finally(), etc. Bookshelf.js's Knex.js does it like this.

@coolaj86
Copy link

+1 for promises. It's really cumbersome to wrap. Particularly I like promises because it makes the error handling logic sooooo much simpler.

@netpoetica
Copy link
Contributor

@coolaj86 Check out https://github.com/leukhin/co-request for using generators instead of promises. Not saying generators are better than promises, but especially if you're interested in error handling, generators can be really helpful. With co-request you will be able to actually wrap your requests in try/catch statements and really throw your errors very easily.

Additionally, co-request is just a wrapper that thunkifies requests for use with the co library and yield statements

@coolaj86
Copy link

@netpoetica as soon as 0.12 lands I'm gonna be all over co like white on rice.

@mikeal
Copy link
Member

mikeal commented Jul 22, 2014

The return value of request() is a stream. There isn't a way to add native promise support without removing streaming, which would be insane.

The dominant compatibility pattern between modules is callbacks, you're going to need to learn to live with that. Both q.denodify and thunkify provide simple and reliable wrappers around standard callbacks for promises and generators respectively. This is a solved problem.

@MajorBreakfast
Copy link

As a side note: It suffices to return a thenable (object with then() method). The thing returned by request may still be a stream. If it has the then() method, it works with co.

Here's the check in co: https://github.com/visionmedia/co/blob/master/index.js#L238

@rayshan
Copy link

rayshan commented Aug 24, 2014

Note that bluebird also provides a way to wrap request:
https://github.com/petkaantonov/bluebird/blob/master/API.md#promisification

var Promise = require("bluebird");
Promise.promisifyAll(require("request"));
// Use request.getAsync(...) not request(..), it will not return a promise

@analog-nico
Copy link
Member

FYI, the latest release of Request-Promise is now 100% compatible with Request plus Promise support.

Although you can even use piping, it is recommended to require('request') for that. Same as when using callbacks the streamed data is buffered until the stream ends to return the whole response body. If you stream large amounts of data this will impact your memory footprint.
Anyway, everywhere you use Request with a callback you can as well use Request-Promise conveniently.

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

7 participants