Skip to content

Added a method to attach "Node-style" callbacks as handlers.#149

Closed
Schoonology wants to merge 1 commit intocujojs:masterfrom
Schoonology:master
Closed

Added a method to attach "Node-style" callbacks as handlers.#149
Schoonology wants to merge 1 commit intocujojs:masterfrom
Schoonology:master

Conversation

@Schoonology
Copy link
Copy Markdown
Contributor

TL;DR: This makes integrating with callback users easier.

I tried to follow the structure as it was apparent, but please ask for updates if I've missed the mark. This met a need of mine, and I assume others may have similar needs.

My use case: I'm building a module for an application that uses promises, but the module is generic enough that I wanted it to accept callbacks, either. Therefore, the API looks thusly:

function clear(callback) {
  var self = this
    , deferred = when.defer()

  self.collection.drop(function (err) {
    if (err) {
      deferred.reject(err)
    } else {
      deferred.resolve(null)
    }
  })

  deferred.promise.both(callback || noop)

  return deferred.promise
}

This way, the callback is treated as both a callback and an errback, each a sibling of any downstream then handlers.

This makes integrating with callback users easier.
@briancavalier
Copy link
Copy Markdown
Member

Hey @Schoonology!

It seems like the use case for something like both is when you have a pre-existing node-style callback that you want to use with both node-style async functions and with promises. Is that right?

Have you had a look at the when/node/function module to see if any of the existing functions in there fit your needs?

BTW, the travis build failure appears to be on their end. We've been seeing weird intermittent travis build failures over the past 2 days. We're looking into it, sorry about that.

@Schoonology
Copy link
Copy Markdown
Contributor Author

All the existing functions in when/node/function are the other direction; they allow a promise user to call node-style-callback-accepting functions. both was intended to go the other direction; to allow a node-style-callback-accepting function to use promises.

Does that answer your question?

Glad the CI failure isn't from this - all the tests passed locally. 😄

@briancavalier
Copy link
Copy Markdown
Member

Ah, I see. Thanks for clarifying.

It seems a bit odd to me for an API to support both passing in node-style callbacks and returning promises. I think I would prefer to advocate one or the other.

That said, how would you feel about taking a slightly different approach. I'd like to keep any node-style callback related code in when/node/*. What if we created a couple new functions, possibly in when/node/function, that can 1) call a node-style callback given a promise, and 2) lift a node-style callback to transform it into a promise-accepting function.

Here's a quick sketch. Def not perfect (see TODO), but good enough for discussion. bindCallback is equivalent to your proposed promise.both:

var when = require('./when');

// ----------------------------------------
// Proposed new APIs for when/node/function

// Function to use a node-style callback as a "dual" promise callback
function bindCallback(nodeback, promise) {
    // TODO: have to consider what the eventual value of the
    // returned promise should be, since nodebacks can't be
    // counted on to return a value.
    return promise.then(success, nodeback);

    function success(value) {
        return nodeback(null, value);
    }
}

// A lift might be useful in some situations as well
function liftCallback(nodeback) {
    return function(promise) {
        return bindCallback(nodeback, promise);
    };
}

// ----------------------------------------
// Using bindCallback

function clear(callback) {
    var self = this
    , deferred = when.defer();

    self.collection.drop(function (err) {
        if (err) {
            deferred.reject(err);
        } else {
            deferred.resolve(null);
        }
    });

    bindCallback(callback, deferred.promise);

    return deferred.promise;
}

@Schoonology
Copy link
Copy Markdown
Contributor Author

Thanks for at least entertaining my desire to placate rather than advocate in this case.

This definitely meets my needs. Shall I update the pull request with the changes suggested? I think the names you selected are suitable.

@briancavalier
Copy link
Copy Markdown
Member

Cool, thanks for being open to other approaches. Yep, feel free to update (force push), or close and open a new one, whichever you prefer!

@briancavalier
Copy link
Copy Markdown
Member

Oops: also could you please rebase/submit against the dev branch? Thanks!

@Schoonology
Copy link
Copy Markdown
Contributor Author

I'll resubmit against dev. I don't think I can switch the existing request.

Stay tuned!

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.

2 participants