ciaranj / node-oauth

OAuth wrapper for node.js
MIT License
2.44k stars 662 forks source link

Support for promises #289

Open danwkennedy opened 8 years ago

danwkennedy commented 8 years ago

Since ES6 is now a thing and support is rolling in, it'd be awesome to be able to have promises returned instead of relying on callbacks (for all the methods that the makes sense). A handy pattern that I've seen is to support both and return a promise if the callback arg is null.

ghost commented 8 years ago

@danwkennedy which promise lib would is the preference? or are promises now part of Node.js in the latest version?

danwkennedy commented 8 years ago

Node 4.0+ has native support for promises with the global Promise object. Some libraries also use helper methods from libraries such as Bluebird that wrap the standard callback functions in promises. The Bluebird route also gives you support for node 0.10 and 0.12 however support for those versions is due to drop in October 2016 and January 2017.

danwkennedy commented 8 years ago

Sorry for the close/open. Hit the wrong button :D

ghost commented 8 years ago

awesome that's very helpful, and it looks like you don't need a flag to use Promises (node docs on es6 features) which is even better.

thanks for the Bluebird suggestion, it looks like it would be a nice way to offer promises without breaking compatibility?

danwkennedy commented 8 years ago

That's correct. A common method for migrating towards promises is to keep the function signatures unchanged and return the promise instead of returning void. You can then return the result to just the promise if the callback is null or to both if the callback is not null. This pattern shouldn't introduce a breaking change for existing callback users and would allow a path to move toward promises (or just use them outright for a new user).

ghost commented 8 years ago

@danwkennedy some good and bad news, I spent yesterday evening trying to make sure all tests pass. The issue is that Vows.js doesn't really work properly with promises. I wasn't sure whether to rewrite the tests completely, try and hack them to work with .asCallback/.nodeify or to use the EventEmitter method that Vows.js allows.

After sleeping on it, I decided to only wrap the public methods that are used and delegate the rest of the methods to the OAuth2 object. Currently we're at 93 tests honoured (I duplicated the oauth2 tests and just changed which library is imported) and 4 broken, the issue is related to the callback not being called.

I've posted a screenshot and the test snippets that are related here: https://github.com/omouse/node-oauth-libre/issues/4

danwkennedy commented 8 years ago

Awesome! Thanks for the update.

Is there any help that I can do? I'm thinking I'll have time to help out in a day or two.

ghost commented 8 years ago

@danwkennedy I need help writing more tests that use the *Async methods and example code that uses the promises-style.

This work has only been done for OAuth2 and needs to be done for OAuth1 as well.

Another issue: from what I see, all http requests go through _executeRequest or _request, but they're private API methods so should they also use Promises style or should only the public api (getOauthAccessToken, get, put, post)?

ghost commented 8 years ago

@danwkennedy oops, forgot to link to the project issue: https://github.com/omouse/node-oauth-libre/issues/4

danwkennedy commented 8 years ago

@omouse Looking over the code, I might be better help keeping the documentation up to date. Do you want all the examples switched to use promises or do you want to add a new set of examples so people can see callbacks/promises?

ghost commented 8 years ago

@danwkennedy new examples using other site APIs would be awesome.

ghost commented 8 years ago

@danwkennedy you'll be interested to know that node-oauth-libre now has promises support: https://github.com/omouse/node-oauth-libre/pull/12 please try out the examples and try it out on a random site that allows oauth 1.0 login and let me know if there's issues with it

niftylettuce commented 8 years ago

very cool @omouse 😄

niftylettuce commented 8 years ago

@omouse I don't see this published on NPM though 😦 -- and the Readme like I mentioned in an issue over there is really inaccurate

ghost commented 8 years ago

@niftylettuce I opened an issue for those just now, didn't see your comment because it was on the PR which is closed:

also I didn't want to publish an alpha version to NPM, until I publish to npm you can use the install instructions in the releases page: https://github.com/omouse/node-oauth-libre/releases/tag/0.9.15-alpha

How To Install And Use

The promises support needs real-world usage and testing to get rid of any kinks.

Install like this:

npm install git+https://github.com/omouse/node-oauth-libre.git#0.9.15-alpha

Then import it into your code:

var OAuth = require('oauth-libre').PromiseOAuth;
var OAuth2 = require('oauth-libre').PromiseOAuth2;

var oauthClient = new OAuth(...);
var oauth2Client = new OAuth2(...);