.done() and .ndone() [was: A useful short-cut for express.js] #86

Closed
ForbesLindesay opened this Issue Jul 12, 2012 · 33 comments

6 participants

@ForbesLindesay
Collaborator

I have been using Q promises with express.js a lot lately, and I'm therefore terminating all promise chains with:

.fail(next).end();

This is because express.js says to pass all unhandled exceptions to next. The end is still necessary in case next throws an exception, which we'd want to just throw globally and exit the program.

It would be really nice if this could become:

.end(next);

Which would just be a shorthand for the first thing?

@kriskowal
Owner

I have considered making .end forward to non-catching callbacks, exiting the purview of the promise system and returning to plain CPS.

@ForbesLindesay
Collaborator

So it would call next(null, promiseValue) if promise just exited normally? I guess some might exect that behaviour, whereas my suggestion only really applies to express, hmm.

@kriskowal
Owner

Oh, not necessarily Node’s CPS style. .end(callback, errback) is what I have in mind, and both callback and errback would not consume exceptions. A different method that explicitly mentions Node would be appropriate for transferring control to a node callback. I don’t think we have one of those yet either.

@ForbesLindesay
Collaborator

ah, yeh I like that suggestion, both for a node-end and for a .end(callback, errback) assuming both could be null/undefined. For a node end, a really nice pattern could then be:

function usePromisesIfYouWant(in, cb) {
    return promisedAPI(in).endNode(cb);
}

That way if cb is undefined, we return a promise for someone to use, but if it is defined, we call it in the normal node style.

@domenic
Collaborator

That way if cb is undefined, we return a promise for someone to use, but if it is defined, we call it in the normal node style.

That is really cool.

@ForbesLindesay
Collaborator

If we agree that both these methods would be useful end and endNode then shall we add some tests and code and get implementing?

Can we agree what the methods should be called? I think end is fairly obvious, but endNode or nodeEnd ? Neither is perfect, but neither is terrible so I'm open to suggestions personally.

@domenic
Collaborator

Oh, not necessarily Node’s CPS style. .end(callback, errback) is what I have in mind

This is the same as WinJS's .done and pretty similar to jQuery's done (which just doesn't have an errback). Should we consider renaming it?

@ForbesLindesay
Collaborator

+1 for done over end if it matches other libraries

@ForbesLindesay
Collaborator

We could do done(callback, errback) and end(nodeCallback) ?

@domenic
Collaborator

I'd say .done(callback, errback), .nodeCallback(nodeCallback), and maybe keep .end() for compat? So far we've been good about keeping node idioms prefixed.

Maybe .nend(nodeCallback), but it's not really the same as .end(), whose purpose is to guarantee errors are never silenced---if you did .nend(function () { }), you'd silence the error.

@kriskowal, thoughts? Agreed on .done()?

@ForbesLindesay
Collaborator

Yes, although .fail(function () {}).end() would silence all errors, nothing can guarantee errors are never silenced. The key point is that .nend(function (err) { if (err) throw err;}) would not silence the error. On the other hand, I'm happy to keep things moderately verbose if that makes it clearer what's going on, so perhaps .nodeCallback or we could go for .ndone as it's very similar to the new .done method

@domenic
Collaborator

After sleeping on it for a few days, I like .done(callback, errback) and .ndone(nodeCallback). We'd deprecate .end (and remove in the next major).

@domenic
Collaborator
@ForbesLindesay
Collaborator

Should it be:

when(promise, fulfilled, rejected).fail(onUnhandledError);

instead of:

when(promise, fulfilled, rejected || onUnhandledError);

The idea is to throw exceptions even if they happen in the fulfilled or rejected handlers.

@domenic
Collaborator

Yup, that makes sense. I need a test "when the promise is fulfilled / and the callback throws / it should rethrow the error in the next turn", and then I need to implement all three of the "it should rethrow the error in the next turn" tests.

@ForbesLindesay
Collaborator

And another for 'when the promise is rejected, and the errback throws, it should rethrow the error in the next turn'

@domenic domenic was assigned Aug 7, 2012
@medikoo

Just to share my experiences. I'm working as well on promise implementation - Deferred, and once I've approached same problems, the way I've solved them, works well on my side:

Currently I have three functions that have same signature as then

  • then(callback, errback) - As we know it, extends the chain by returning newly created promise
  • aside(callback, errback) - Returns input promise, does not create a new one and doesn't end the chain. It's useful when we want to return promise for further processing but also need to do something with result on the side.
  • end(callback, errback) - Ends promise chain. doesn't return anything and doesn't create any new promise. Both callbacks are optional. If errback is not provided eventual error will throw. As I think of it now done name also describes very well this case.

So currently more often I use end or aside than then as in many cases I don't need promises returned by then, it's much cleaner and definitely more performant.

For bridging Node CPS I have cb function, that takes callback, and calls it in Node.js style. Callback is optional and if provided it is run in next tick the earliest, this function also returns input promise, so it's handy when building hybrid functions that both return promise and handle Node.js style callback, e.g.:

var asyncFunc = function (a, b, cb) {
    // ...
    return promise.cb(cb);
};
@ForbesLindesay
Collaborator

How does this relate to nend etc. ?

@medikoo

@ForbesLindesay not sure what you're asking, but as I mentioned for transforming back to Node CPS style Deferred has promise.cb.

When talking about nend specifically, I see that nend additionally creates and returns new promise which would be resolved right before callback is called. I have problems understanding what use case does it address, as technically all that information (if needed) is already accessible on input promise.

@domenic
Collaborator

My original plan was for ndone to replace nend. But as @medikoo points out, nend returns a promise. This, I believe, is designed to enable APIs like that of node-flurry, like so.

@kriskowal
Owner

nend(cb) does return a promise but it’s only to make it easier to test. I might remove it and use waitsFor and a spy in the spec.

@kriskowal
Owner

@domenic I need to read this thread top-to-bottom again. I haven’t made a decision, and have not committed to nend and done and ndone are still on the table.

@ForbesLindesay
Collaborator

I do like the idea of having an easy way to make my public interfaces directly support node-style and promise-style use just by either passing or not passing a callback.

@domenic
Collaborator

To summarize and bring in some thoughts:

.done(f, r, p)

Acts like then except that it does not return anything, and causes a next-tick-throw upon rejection. Equivalent to today's .then(f, r, p).end(). Replaces .end() since .done() is equivalent.

Matches WinJS. Similar to jQuery's .done(f[, f, f, ...]), which notably returns the original promise.

.ndone(cb)

Was envisioned as .then(function (x) { cb(null, x); }, cb).end(), or as .end() alone if no cb is passed.

.nend(cb)

Check out the implementation

Things to note:

  • If cb is falsy, it will return the original promise. This is useful as we'll see below.
  • The use of nextTick escapes the handler-catchers, so that errors thrown in the callback jump to the top of the stack (Node-style).
  • It's not very end-ey: without a callback, it acts nothing like end.

On nend or ndone enabling dual APIs

nend currently enables dual APIs like so:

function asyncFunc(a, b, c, cb) {
    return promise.nend(cb);
}

This is very nice.

If we were to move to ndone as part of a move to done a naive translation no longer works, since

function asyncFunc(a, b, c, cb) {
    promise.ndone(cb);
    return promise;
}

will throw-in-next-tick if promise ends up being rejected, denying any promise-using callers a chance to handle rejections. This makes me think ndone is pretty useless.

Recommendation

Whereas,

  • nend as it exists is awesome, but it is named confusingly since it doesn't behave like end at all;
  • done is still a good idea and a good replacement for end;
  • ndone turns out to be useless;

I think we should replace end with done and do nothing else. Keep nend named as it is, since there will no longer be a end to confuse matters. And forget about ndone.

@kriskowal
Owner

nend needs a different name. I’ll buy done as you propose.

@kriskowal
Owner

nend stands for "if the user provides a nodeback, transfer control to Node-style continuation passing. otherwise, return the promise, so this function can be used by both promise consumers and nodeback providers". That’s a mouthful and might be hard to capture, but I’ll buy up to three words and no more than 80 characters.

@kriskowal
Owner

Some potential seeds for the name:

  • cb
  • toNode
  • toNodeback
  • maybeToNodeback
  • ambidextrous
  • ambi
@tlrobinson
  • hermaphrodite
@kriskowal
Owner
  • honeyBadger
  • nodeCompatible
@domenic
Collaborator
  • nodeify
  • maybeNodeify
@kriskowal
Owner
  • supportNode
@ForbesLindesay
Collaborator

+1 for supportNode
+1 for nodeify
+0.5 for toNodeback

In the use case described of "I wish to expose an api that's familiar to people used to node style callbacks" I'd go with supportNode as the hands-down winner.

For the alternative case of working with things like express.js where you must call a callback as a node.js callback, it seems a little counter-intuitive to call it supportNode, which makes me favour something like nodeify.

toNodeBack seems nice and un-offensive, but could be confused with Deffered's makeNodeResolver.

@domenic domenic added a commit that closed this issue Oct 13, 2012
@domenic domenic Add `.done` as a replacement for `.end`.
- Acts as a better `.end`, taking the usual fulfillment, rejection, and progress handlers. Essentially equivalent to `.then(f, r, p).end()`.
- Closes #86.
0c67480
@domenic domenic closed this in 0c67480 Oct 13, 2012
This was referenced Oct 13, 2012
@domenic domenic added a commit that referenced this issue Oct 16, 2012
@domenic domenic Add `.done` as a replacement for `.end`.
- Acts as a better `.end`, taking the usual fulfillment, rejection, and progress handlers. Essentially equivalent to `.then(f, r, p).end()`.
- Closes #86.
16c913d
@rstacruz

For those coming in via a Google search, let me save you some reading:

  • This has been merged and is now called promise.nodeify().
  • Calling .nodeify(done) is the the equivalent of .then(function(data) { done(null, data); }, done).
  • There's also promise.done(), where .done(x,y,z) is the same as .then(x,y,z).done().

Express.js example:

function (req, res, next) {
  Q.when(Posts.findAll())
  .then(function() {
    // Do things
  })
  .nodeify(next);
});

Mocha.js example:

it("should work properly", function(done) {
  Q.when(Posts.findAll())
  .then(function(posts) {
    assert.equal posts.length, 40
  })
  .nodeify(done);
});
@thanpolas thanpolas referenced this issue in sequelize/sequelize Oct 7, 2013
Closed

Implement asynchronicity in getters / setters #965

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment