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 convenience methods like .always() #13

Closed
briancavalier opened this issue Jan 12, 2012 · 31 comments
Closed

Add convenience methods like .always() #13

briancavalier opened this issue Jan 12, 2012 · 31 comments
Assignees
Milestone

Comments

@briancavalier
Copy link
Member

Currently, when.js's promise provides .then(), so if you want to register the same callback to run whether the promise is resolved or rejected, you have to pass it explicitly as both the callback and errback to when() or .then(). Both jQuery Deferred, and Q's promises provide convenience methods for this.

Here's how you have to do it with when.js currently:

promise.then(callback, callback);
// or
when(promise, callback, callback);

That's not horrible, but this seems like a nice convenience:

promise.always(callback);
// or
when.always(promise, callback);

It might be interesting to provide both promise.always(callback) and when.always(promise, callback) so that when.always() can be used with other promise implementations. Also, since something like promise.always() is not part of the Promises/A spec, relying on it would mean tying your code to when.js's promise implementation. Using when.always(promise) is a bit more decoupled.

Seems like this also opens the door for a method that registers only a rejection handler. For example, to do that now:

promise.then(null, errback);
// or
when(promise, null, errback);

Which is kind of ugly, and might cause a "WTF". So, could provide something like:

promise.rejected(errback);
// or
when.rejected(promise, errback);

This seems like a much more limited use case to me, though.

@sifu
Copy link

sifu commented Jan 13, 2012

an .addErrback() or .fail() (see eg. http://api.jquery.com/deferred.fail/) could be a nice addition. there is nothing you couldn't do with .then(), but i use it all the time to make code more readable.

@sifu
Copy link

sifu commented Jan 13, 2012

oh and +1 for .always()

@briancavalier
Copy link
Member Author

Hey @sifu, thanks for the feedback.

I do have a concern about .addErrback()/.fail(), and maybe it applies to .always() as well. Do you think there will be confusion between Promises/A semantics and jQuery Deferred semantics? For example:

var success, failed;
success = failed = function(val) {
    return val + 1;
};

function log(val) {
    console.log(val);
}

// First, jQuery Deferred
var d = $.Deferred();

// This prints 1 because jQuery Deferred isn't Promises/A
// then(), fail(), etc. return `this`, and all handlers receive 1
// no matter where in the "chain" they fall
d.then(success).fail(failed).fail(log);
d.reject(1);

// Now when.js
// Suppose when.js's promises support .fail() - would be similar for always()
d = when.defer();

// This prints 2 because of Promises/A forwarding
// then(), fail() return a new Promise that receives the result
// of the previous Promise
d.then(success).fail(failed).fail(log);
d.reject(1);

So, in the second example, some may feel that it reads as if you are registering the success and failed handlers with the same promise, but by Promises/A semantics, you really aren't. The deferred is rejected with the value 1, which is received by the first fail() handler, which returns a new result of val+1, which would be forwarded to the next fail() handler, thus logging 2.

Maybe it's just a documentation issue, but I do want to be very careful here not to introduce any APIs that might cause a lot of confusion (and possibly have to rip them out later!)

Thoughts?

@sifu
Copy link

sifu commented Jan 15, 2012

i think i agree with your concerns.

my problem is, that i'm so used to the original deferreds API, because i use deferreds since about a decade (with twisted-python) and promises since about 1.5-2 years, that my thoughts on that matter are very biased.

btw. always thought the deferred methods .addCallback, .addErrback, addBoth, ... are way clearer than the promised equivalents like .then and all the rest.

i also think i withdraw my "+1" for .always( ) :)
i just figured out, that i use it quite often, but only for async tests inside qunit. and that one use-case is not enough to vote for it.

it is a shame that jQuery opted for a non-standards API. since jQuery is so common, their API will be the one most javascript programmers will get familiar with. :(

@briancavalier
Copy link
Member Author

Thanks, really appreciate all the feedback so far, @sifu.

This is a tough issue, for sure--trying to provide convenience while avoiding confusion, and I totally agree that jQuery's choices (and popularity) make it even tougher. The really unfortunate thing about jQuery Deferred, IMHO, is that they chose to look like Promises/A, but not act like it, i.e. typeof $.Deferred().then == 'function', so you can't rely on the feature test.

I think we need more discussion/thinking on this issue before we know what is best. I'll keep the issue open until we decide something, but we probably won't implement anything any time soon.

@sifu
Copy link

sifu commented Jan 16, 2012

btw. i don't know if you know the reasoning behing jquerys .pipe() vs .then():

they had concerns, that always returning a new promise from .then() would be a performance problem. and they talked to the dojo guys about how to resolve that problem and figured that a new method .pipe() would be the best solution. thats also the reason, why they introduced this method one version later.

imho a better way would have been to do it the other way around. provide a method, that doesn't return a new promise (which you can opt in to use if you have performance issues), but implement .then() like it is supposed to be.

@briancavalier
Copy link
Member Author

I had heard that once before, but can't remember where. Pretty interesting. I know jQuery has their own goals and priorities, so I can certainly appreciate that performance might be a top priority. But, yeah, I do agree that "doing it the other way around" may have been a better approach, at least from the perspective of being friendly to Promises/A.

I think (hope) the fact that when.js assimilates jQuery Deferred now gives jQuery devs a good option if they want Promises/A. They can use $.Deferred if they want, but then can call when() if they want to guarantee forwarding. They can also return when($.Deferred()) if they want to be nice and return compliant promises from an API.

We've been using when.js as a part of wire.js (https://github.com/briancavalier/wire), and also in application-level code, on a big project (300+ AMD modules) for the past year, and we haven't yet had any perf problems that we could attribute to promises. I don't have any hard numbers, but I feel like the performance is at least good enough that application code will probably be more of a perf hit than promises.

@briancavalier
Copy link
Member Author

I think I'm back to feeling like .always() and .fail() might be a good thing. It seems like calling .fail(...).fail(...), like in my example above, might be pretty rare. I can't think of when I've ever done that. A problem that might trick devs more often might be something like this:

when(computeResult(),
    handleResult,
    handleFailure);

In this case, if computeResult() fails, then we're ok--handleFailure will be called. However, if handleResult fails, handleFailure will not be called since promises guarantee that either the callback or errback will be called, but never both. It's basically equivalent to this try/catch, which makes the situation a bit more obvious:

var result;
try {
    result = computeResult();
} catch (e) {
    handleFailure(e);
}

handleSuccess(result);

So, if when.js provides a .fail(), it might be provide a more intuitive way of dealing with that situation:

when(computeResult(),
    handleResult)
    .fail(handleFailure);

In that case, because of forwarding, handleFailure will be called when either computeResult or handleResult throws or rejects. Of course, you can do that with just .then(), but it's uglier, imho:

when(computeResult(),
    handleResult)
    .then(null, handleFailure);

I'm still torn, but it seems like the right thing to do is to pick the scenarios that are most likely to cause headaches, and provide nice ways of dealing with those, and then let documentation handle other, less-likely scenarios (like .fail().fail()).

Any thoughts?

@sifu
Copy link

sifu commented Feb 2, 2012

huch, i just realize, that i never thought much about, if handleFailure gets called in case handleResult has an error (when you add both to .then at once). i almost never attach both, and always have a single error handler at the bottom of my promise chain.
but on the first glance one definitely might think the handleFailure would be called.

so a .fail() would make more people use success and error handler separately and therefore avoid the issue.

and i can't recall a single scenario, where i wanted to construct something to the liking of .fail().fail().

i again have to say, that i preferred .addCallback .addErrback, back in the twisted-python days :) why didn't promises continued to use this naming convention... :/

btw 300+ modules sound big. wow! what does the app do?
and i also never had an performance issue related to promises/deferreds.

@briancavalier
Copy link
Member Author

Coincidentally, I just got tripped up by this, and had to end up writing some code that looks like:

when(doStuff(), doOtherStuff /*, cleanup */) // don't call cleanup here, since doOtherStuff may fail!
    .then(cleanup, cleanup);

It'd sure would have been nice to write that like this:

when(doStuff(), doOtherStuff)
    .always(cleanup);

I'm curious about addCallback and addErrback. Do you feel like they are more intuitive solely because of their names, or because they promote writing code like:

promise.addCallback(handleSuccess);
promise.addErrback(handleFailed);

Do Twisted's Deferreds/Promises support chaining and forwarding? If you have pointers to some good examples or tutorials, I'd love to take a look and get a feel for how Twisted's Deferreds work.

The 300+ module app was one that @unscriptable and I worked on together starting back in November 2010. The company is LifeIMAGE, and they do cloud-based medical image sharing. The app was their front-end for uploading, viewing, managing, and securely sharing medical images in a workflow across doctors, hospitals, etc. It was highly visual and interactive, so most of the modules were Views that had sub-modules for their HTML template, CSS, JS view controller, and i18n. Cool stuff, and a really fun project.

@sifu
Copy link

sifu commented Feb 2, 2012

i prefere addCallback / addErrback because of both,

and twisteds deferreds work differently to promises in that they don't return a new deferred/promise like .then does, but the state is passed on to the next callback/errback, so the effect is similar and easier.

i don't use twisted anymore, but i think http://twistedmatrix.com/documents/current/core/howto/defer.html is still the best explanation.

@briancavalier
Copy link
Member Author

Very interesting, and thanks for the link--I'll definitely take a look.

@ghost ghost assigned briancavalier Feb 9, 2012
@briancavalier
Copy link
Member Author

Very good read about twisted deferreds! Thanks again for the link.

I'm putting this back on the table for 1.1.0, now that I think the benefits outweigh the drawbacks, and the use cases where always() is helpful seem to be more likely than ones that might get you into trouble.

@briancavalier
Copy link
Member Author

@sifu any thoughts on fail() vs. or() ?

when(stuffHappens, handleIt).or(recoverFromFailure);

// vs.

when(stuffHappens, handleIt).fail(recoverFromFailure);

To me, fail seems like a verb--as if it causes a failure when I call it :)

@sifu
Copy link

sifu commented Feb 10, 2012

i'm not a english native speaker, so i don't necessary associate the same things to words.
but .or seems to general. for example, i would be confused if i see a whole promise chain with single .or at the end.
.fail would be at least familiar to anyone using the jQuery API.
if you don't care at existing APIs, how about something like .onFailure?

@johnyanarella
Copy link

I agree - fail sounds like a verb, and implies that invoking that method would trigger a failure.

How about fails?

From a fluent API perspective, I think it reads very well:

when( computeResult() ).fails( recoverFromFailure )

which matches the way you earlier described the behavior in plain English.

@briancavalier
Copy link
Member Author

Cool. Yeah, fails does read nicely. Here's my previous example, using both or and fails

when(stuffHappens, handleIt).fails(recoverFromFailure);

// vs.

when(stuffHappens, handleIt).or(recoverFromFailure);

I like both of those. And here's an example using .then():

// awkward or ok?
promise.then(handleIt).fails(recoverFromFailure);

// vs.

promise.then(handleIt).or(recoverFromFailure);

Hard for me to say which or those I prefer right now ... have to stare at them for a while :) Any more thoughts after seeing those two examples?

@briancavalier
Copy link
Member Author

Here are the same examples using @sifu's onFailure suggestion for comparison with or and fails. IMHO, these read pretty nicely, too:

when(stuffHappens, handleIt).onFailure(recoverFromFailure);

and using .then():

promise.then(handleIt).onFailure(recoverFromFailure);

@jonnyreeves
Copy link

Just to chime in on the naming, I think or is close, but otherwise might fit the bill better?

when(stuffHappens, handleIt).otherwise(recoverFromFailure);
promise.then(handleIt).otherwise(removeFromFailure);

@briancavalier
Copy link
Member Author

Ah, very interesting! It reads nicely, and seems intuitive. promise.always() just landed in v1.1.0, but we held off on or because we weren't 100% convinced on the name.

I'd love to hear what other folks think of otherwise vs or.

Thanks for the suggestion, @jonnyreeves

@unscriptable
Copy link
Member

+1 for "otherwise"

@johnyanarella
Copy link

+1 for "otherwise" - I am inclined to mirror that API method name in Deft JS's promises

@briancavalier
Copy link
Member Author

Since .always() is released, I've opened a new issue for .otherwise(), or whatever we end up calling it: #29

@briancavalier
Copy link
Member Author

promise.otherwise(errback) just landed in the dev branch, and will be in the upcoming 1.2.0 release. Thanks again to @jonnyreeves for suggesting the name "otherwise"!

@johnyanarella
Copy link

Over in the Deft JS project, we also adopted otherwise() in our Deferred/Promise API.

Today, I had a developer report an issue with otherwise() that made me realize that this particular name implies something other than what this method actually does.

In our discussions here and elsewhere, we considered otherwise() to just be a shortcut for promise.then( null, errback ), however it reads as if it is a control statement and initiates a new value resolution branch.

Consider:

   var promise = check();
   promise.then( proceed );
   promise.otherwise( recover ).then( proceed );

The developer in question was surprised that proceed() was executed twice when check() resolves. So was I, until I traced through the way that Promise resolution is supposed to propagate. The Promise returned by otherwise() resolves when the original promise resolves.

The issue is that otherwise() implies control flow; it reads as if the subsequent then() is 'guarded' by the otherwise() and wouldn't be executed unless the original promise was rejected.

So this raises some questions.

  1. Has it turned out that otherwise was the wrong name after all? Perhaps addErrback() more accurately conveys the actual behavior after all.

  2. Should a Promise API provide control flow statements or does that deviate too far from its focused 'future value' intentions into asynchronous operation control flow management territory instead?

  3. If so, should otherwise() behave as it reads, and only resolve when the original promise is rejected? What should happen when the original promise is resolved? Should it cancel? (Otherwise the promise will remain in a pending state where callbacks won't be released - leaking memory.)

@briancavalier
Copy link
Member Author

Hey John, that's certainly an interesting case of perceptions!

I've not run into any confusion with otherwise() yet, although I do agree that the .otherwise().then() construct is a strange one. Seeing .then(proceed) twice in the same promise chain, so close to one another was def a red flag for me. Interestingly, if I understand your example correctly, the last line is sufficient by itself:

check().otherwise(recover).then(proceed);

To my eyes, that is very readable and obvious, but I also know that I look at promises every day, and so I'm not always the best judge of what is intuitive to other folks!

Do you think this could be a learning curve and/or documentation issue, rather than something intrinsically misleading about otherwise()?

Another thing to consider is that although otherwise() isn't a standard Promises/A method, there are now at least 3 implementations that use it (when.js, DeftJS, and the upcoming dojo 1.8 release), and they all currently behave in the same way. That kind of consistency should help with general learning curve a bit as promises become more widely used.

As for introducing control flow, I think it would be interesting to explore. I'm not sure it belongs in the promise implementation itself, but rather feels to me like something that should be built on top of promises. If it's integrated into the promise implementation, I think it would have to be done very carefully so as not to break Promises/A interoperability.

Of course, if your promises are never exposed in a way that they might be consumed by code that is expecting Promises/A, interoperability is less of an issue. For when.js, tho, that's not an option, since many folks are using it alongside other promise implementations.

So, I guess to sum up: 1) right now I wonder if this is a documentation or learning curve issue, and 2) I think promise-based control flow constructs could be very useful, but it feels like something higher level that should be built on top of promises.

Thoughts?

@motorro
Copy link

motorro commented Jul 22, 2012

Hello everybody!

I'm a developer John is taking about... :)
In fact I was looking to some task / control flow framework and Deft implementation appeared to be the kind.
Here is a full abstract of control flow I was after:

        check(false)
            .then(proceed)
            .otherwise(recover)
                .then(function(){return check(true)})
                    .then(proceed);

For me it seems expressive - the chain describes the full control flow I use for loading init data:

Init()
    .then(proceed)
    .otherwise(login)
        .then(init)
            .then(proceed)

The app tries to init and asks for login if not authorized. App reignites after login. If we have an active session already we go the first 'then' and login does not appear.

@briancavalier
Copy link
Member Author

Hey Nikolai! Promises, and specifically Promises/A, aren't really intended, by themselves, as a tool for setting up and running complex control flow graphs. They're really intended to manage future values, and observers of those values.

That said, it's entirely possible to build more complex asynchronous control flow on top of promises. I built an asynchronous state machine on top of when.js promises, and a simple polling utility

otherwise() is actually just an alias for .then(null, errorHandler), and isn't intended to imply deep control flow branching. So, these two are equivalent:

check().otherwise(recover).then(proceed);

check().then(null, recover).then(proceed);

They are both analagous to the following synchronous code:

var value;
try {
    value = check();
} catch(e) {
    value = recover(e);
}

proceed(value);

So, while then() and otherwise() do allow you to register separate success and error handler callbacks, they don't imply deep control flow branching.

However, I think it's possible to accomplish your example without layering any additional control flow on top of promises:

init()
    .otherwise(function() {

        // If init() rejects, we need to recover before proceeding

        // This inner function represents a sub-flow, and can also
        // return a promise.  Subsequent *outer* callbacks (see proceed below)
        // will only be invoked after the returned promise resolves.

        return login()
            .then(init);

    })
    .then(proceed);

It's possible to reduce the nesting a bit by abstracting the recovery:

init()
    .otherwise(recover)
    .then(proceed);

function recover() {
    return login()
        .then(init);
}

Both of those basically say "try to initialize, and if that fails, recover by logging in and then initializing. Once successfully initialized, proceed". Again, it is equivalent to the synchronous try/catch above.

That still may be slightly more nesting that you'd like, and if your control flow graph has lots of additional branches and sub-flows, it could become a bit more unweildy. So, depending on your needs, it may be worth building simple simple promise-based control flow constructs.

I hope that helps, and feel free to keep the discussion going if you have more questions.

@motorro
Copy link

motorro commented Jul 24, 2012

Hello Brian!

Thank you for your explanation!
In fact I have already solved the problem exactly the way you propose (by introducing an extra function).
I had a discussion with John on that case and he has also proposed that way.
So, as you say, I'm trying to build a task management on top of promises - that suits me right.
The reason I've posted here was to somehow show the "expressiveness" of the control flow implementation effort :O)
I'll also try your "robo" framework - thank you for the clue!
Regards!

@briancavalier
Copy link
Member Author

No problem, Nikolai, glad you found a solution, and thanks @johnyanarella for bringing this discussion up.

Robo is definitely a work in progress, so the usual caveats apply :) If you try it out, though, I'd certainly love to hear what you think.

@johnyanarella
Copy link

Great responses, @briancavalier. Your views on this mirror mine - and to the extent that there's a learning curve associated with the proper application of the otherwise() method, your responses in this thread will be a great place to point people in the future. I wish GitHub had a reputation point system of some kind, if it did, I'd be throwing some your way for the in-depth explanations you provided here.

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

6 participants