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

Expose Container constructor #108

Closed
wants to merge 2 commits into from
Closed

Expose Container constructor #108

wants to merge 2 commits into from

Conversation

d6u
Copy link

@d6u d6u commented Nov 29, 2014

So we can use bluebird's Promise.promisifyAll[1*] method like so:

var Promise = require('bluebird')
Promise.promisifyAll(Docker.Container.prototype)

// later on
Docker.createContainer(function(container) {
    container.startAsync().then(function () { ... }); // startAsync is one of the promisified methods
})

1 - https://github.com/petkaantonov/bluebird/blob/master/API.md#promisepromisifyallobject-target--object-options---object

So we can use bluebird's `Promise.promisifyAll` method
@apocas
Copy link
Owner

apocas commented Dec 10, 2014

Not sure about this one.

If we going to expose things, it is going to be everything or nothing.
Don't want to create a big mess in terms of usage.

Now, I'm not sure if we should expose them for this use case.

@d6u
Copy link
Author

d6u commented Dec 11, 2014

@apocas I updates the request, now it exposes all constructors (let me know if I missed any).

Basically what Promise.promisifyAll will do is to convert traditional node.js callback API to promise API. The the following:

docker.listImages({}, function (err, images) {
  docker.createContainer({}, function (err, container) {
    container.start(function (err) {
      container.exec({}, function(err, exec) {
        exec.start({}, function (err, stream) {
          // ...
        });
      });
    });
  });
});

can becomes:

docker.listImagesAsync()
  .then(function (images) {
    return docker.createContainerAsync();
  })
  .tap(function (container) {
    return container.startAsync();
  })
  .then(function (container) {
    return container.execAsync();
  })
  .then(function (exec) {
    return exec.startAsync()
  })
  .then(function (stream) {
    // ...
  })
  .catch(function () {
    // catch all errors here
  });

which is flat when nesting a lot of callbacks. Without exposing constructors, I am currently doing:

Promise.promisifyAll(Docker.prototype);

var _getContainer = Docker.prototype.getContainer;

Docker.prototype.getContainer = function () {
  var container = _getContainer.apply(this, arguments);
  if (!container.startAsync) {
    Promise.promisifyAll(Object.getPrototypeOf(container));
  }
  return container;
}

@apocas
Copy link
Owner

apocas commented Dec 11, 2014

@d6u
Copy link
Author

d6u commented Dec 11, 2014

dockerode-promise is nice, but bluebird is more popular and provides more useful API. Do you think it would be more consistent if I create another npm package, like dockerode-promise, to wrap around dockerode?

@apocas
Copy link
Owner

apocas commented Dec 11, 2014

Yeah exactly.
I would prefer to push that into something to like "dockerode-bluebird" leaving dockerode lighter and focused just on abstracting Docker :)

@d6u
Copy link
Author

d6u commented Dec 12, 2014

I see, thanks.

@d6u d6u closed this Dec 12, 2014
@amitaibu
Copy link

amitaibu commented Apr 9, 2015

@daiweilu I believe the result of this is https://github.com/daiweilu/Dockership , correct?

@d6u
Copy link
Author

d6u commented Apr 10, 2015

@amitaibu Not exactly, https://github.com/daiweilu/Dockership/blob/master/lib/promisified/docker-promisified.js is the file that convert Dockerode into promise style.

@amitaibu
Copy link

👍

@apocas apocas mentioned this pull request Jul 8, 2015
@apocas apocas mentioned this pull request Aug 23, 2015
@apocas
Copy link
Owner

apocas commented Sep 3, 2015

Awesome! There were a few people asking for this 👍

@sheerun
Copy link

sheerun commented Sep 1, 2016

Please include promise interface into this repository. Promise-wrapping packages quickly become out of date...

@KristerV
Copy link

+1 sheerun. Why are callback still a thing...

@nwalters512
Copy link

I'd like to second this. All of the "wrapped" versions haven't been updated in over a year. Trying to do anything complex with this quickly leads to callback hell.

@apocas
Copy link
Owner

apocas commented Feb 28, 2017

I will expose all the constructors to make all of you promise lovers happier :)

I'm not saying that what we have right now leads to a callback hell, I continue to say that really depends on the writer's habits, schools, patterns, taste, ...

Just want Dockerode to be a really good base structure to everyone :)

@apocas
Copy link
Owner

apocas commented Mar 2, 2017

Here we go:
8585228

Happy to announce that I implemented a promises based interface :)
Please comment and contribute, I really want feedback before even thinking on publishing it.

https://github.com/apocas/dockerode#promises-yet-to-be-published

@sheerun
Copy link

sheerun commented Mar 2, 2017 via email

@apocas
Copy link
Owner

apocas commented Mar 2, 2017

Feedback needed: #339

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

Successfully merging this pull request may close these issues.

None yet

6 participants