Node-Virtualization / node-virtualbox

A JavaScript Library for Interacting with VirtualBox
MIT License
259 stars 68 forks source link

Replace callbacks with promises #20

Closed michaelsanford closed 5 years ago

michaelsanford commented 9 years ago

Get with the times: shall we replace all those callbacks with promises?

azer commented 9 years ago

is it possible to have promises as a higher level abstraction layer on top of existing api ?

michaelsanford commented 9 years ago

I thought about that, too, @azer. (This is a suggestion from an embedded //todo that I wanted to open up for community discussion.)

One problem that promises don't solve directly is that the resolution/success callback does not indicate that the intent was actually successful, just that the shell fired and returned.

Promises might be raised to a higher level abstraction — as you suggest — and be made more useful by chaining intents to their consequences, only resolving/rejecting once the complete intent can be verified one way or the other. For example, the promise from vb.start will resolve after start was fired and the VM can be verified to be running, and would reject if the vm is verified to have failed to start ($?, timeout, etc).

Thoughts?

Stylizit commented 7 years ago

Hi,

I am writing a toolkit to automatically spin up VMs and launch layout tests with ievms and virtualbox, so I had to automate quite a bunch of commands to pass to the Guest VM.

I ended up coding a very light wrapper on top of the existing API to have better flow control with Promises. Maybe we could do something along the lines of :

function vboxwrapper(fn, args) {
  return new Promise(function (resolve, reject) {
    var handler = function () {
      var err = arguments[arguments.length - 1];
      if (err)
        reject(err);
      resolve(args);
    };

    virtualbox[fn].call(this, args, handler); 
  });
}

It would allow you to keep the existing API and call the Promise-based API like this

.vboxwrapper('start', [{vm:"machine_name"}, true]).then(function(res){...})
.vboxwrapper('snapshotTake', [{vm:"machine_name"}, {vm:"snapshot_name"}]).then(function(res){...})
...

What do you think?

Stylizit commented 7 years ago

Let me know if you're ok with this and if you'd like me to help, I'd be happy to send a PR.

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not received any attention in 90 days. It will be closed in 14 days if no further activity occurs. Thank you for your contribution! (I'm a bot.)

stale[bot] commented 5 years ago

This issue has been automatically closed due to inactivity. (I'm a bot.)