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 #17

Open
mribichich opened this issue Apr 20, 2018 · 6 comments
Open

Add support for Promises #17

mribichich opened this issue Apr 20, 2018 · 6 comments

Comments

@mribichich
Copy link
Collaborator

What do you think about supporting Promises?

If you dont want to remove callbacks for backwards compatibility, there's a method that could be use to support both

Basically if the user doesn't provide a callback a Promise is return, that way the lib can support both

There's this library that implements this really easy, and with the advantage that's really easy to remove callbacks and this lib in the future if you want to.

The lib is nodeify

Simple sample:

var Promise = require('nodeify').Promise;

function myAsyncMethod(arg, callback) {
  return new Promise(function (resolver) {
    //do async work
  })
  .nodeify(callback);
}

So in the future if you want to remove callbacks, all you have to do is remove:

  • var Promise = require('nodeify').Promise;
  • .nodeify(callback);

what do you think? Promises will make much more friendly and es2015, and allow the use of async/await

@bulentv
Copy link
Owner

bulentv commented Apr 20, 2018

Sure, why not have Promises too if this is the time to switch to es2015.

Feel free to start implementing it but as you pointed out, we need to continue supporting the old API. So I put some deprecation warnings for renamed methods so that they can stay till the v0.3

We can do the same on promises (if possible?)

@mribichich
Copy link
Collaborator Author

I'll wait until you finish the transformation to es2015. Or you 're not planning on doing it for now?

Since the change to es2015 or Promises is a huge one, merging it will be hard y we change them separately. I can help you with es2015 if you want.

@mribichich
Copy link
Collaborator Author

I see you already did the transformation. great!

@bulentv
Copy link
Owner

bulentv commented Apr 22, 2018

Yes I did but of course it needs to be tested on a real system. You can directly push your changes. We can test it little more before implementing Promises, maybe in a new branch first?

@mribichich
Copy link
Collaborator Author

I can test were I'm using it. I dont use every function but I could test them.

I will try to set a couple of unit tests for test the callback/promise setup

@caobo171
Copy link

caobo171 commented Nov 8, 2019

I Changed it in my repo to using asyn / await !!

Check my repo , I've already fixed many bugs :
https://github.com/caobo171/node-zklib

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

3 participants