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

Make methods return Promises #9

Closed
eorroe opened this issue Nov 13, 2015 · 15 comments
Closed

Make methods return Promises #9

eorroe opened this issue Nov 13, 2015 · 15 comments

Comments

@eorroe
Copy link

eorroe commented Nov 13, 2015

Could you make the methods return a promise, so that devs who want to use Promise#then method could use that over callbacks, so the callbacks would be optional. It'll allow much more flexibility and get rid of spaghetti code

@jaredreich
Copy link
Owner

Thanks for the suggestion. Do you mean spaghetti code could occur within a notie.confirm call? Not sure if this should be implemented as it currently follows the same format for callbacks as native javascript functions such as:

setTimeout(function() {
    // do stuff
}, 3000);

or jQuery functions such as:

$('#element').on('click', function() {
    // do stuff
});

@eorroe
Copy link
Author

eorroe commented Nov 13, 2015

Yes its fine to have callbacks, but I"m sure devs will like having the use of Promises as well, looking at future native implementations on the web almost all will be using Promises. Not just to rid of spaghetti code, but because promises allow for referencing future values etc

@andylima
Copy link

+1

@andylima
Copy link

@jaredreich Check out this article to see what we mean:
https://www.twilio.com/blog/2015/10/asyncawait-the-hero-javascript-deserved.html

...And it shows exactly how to promisify a function. :)

@jaredreich
Copy link
Owner

Okay cool thanks for the info, I'll look into this.

@singerxt
Copy link

I added support for Promises for "confirm function" please review.
#14

@jaredreich
Copy link
Owner

Thank you very much I'll take a look.

@taromero
Copy link

I would just add adapt #confirm to follow JS callback style convention, which is for the callback to take 2 arguments (error and result, in that order), and always call the callback function after the async operation (user I/O in this case) is completed.

An async function that takes a callback can easily be adapted to return promises by the library user. At least Bluebird (a widely adopted Promise library) provides methods for promisification.

The problem with the current implementation is that the promisified #confirm method will never be fulfilled if the user clicks on 'No', because the callback function is never called on that case.

I think that the same applies for #input.

@eorroe
Copy link
Author

eorroe commented Nov 19, 2015

Make the promise reject?

@taromero
Copy link

I thought about the same the moment after writing the comment.

I don't know what would be better, to reject the promise when the user clicks 'No', or just send a different value as the callback result parameter (like 'yes' if the user confirmed (clicked on 'Yes') and 'no' if it didn't (clicking on 'No')).

If this is going to be translated to promises, it will have to handle the case when the user clicks on 'No', by fulfilling the promise in some way (rejecting on fulfilling with a particular value).

@eorroe
Copy link
Author

eorroe commented Nov 19, 2015

Yea I thought about that, too but didn't put input. Yea it makes sense to me now. Its better to pass the value to the promise since its still a callback, its more work for the dev to check the value, but they get the convenience of promises.

@carcinocron
Copy link

👍

I think most libraries will return a promise and also accept the callback.

But it looks like IE11 and OperaMini users will need a polyfill:
http://caniuse.com/#feat=promises

So that might conflict with the no dependencies goal.

@connorjburton
Copy link

I've ran into the same issue with polyfills and ended up putting small polyfills in the notie.js file. Not sure how big the promises polyfills is though.

@carcinocron
Copy link

How many polyfills does notie.js use/need? I think it would be better to include those as a separate optional file so that we're not including the same polyfills twice. I think most modern web developers are using tools like grunt/gulp/webpack and wouldn't appreciate it if every single library included its own copy of es6-promise, or forced me to use bluebird when I need Q's small size or force me to use Q when I need bluebird's performance.

@connorjburton
Copy link

Two at the moment, and yeah I'd agree on separating them out.

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