feat($q): added support to promise notification

[caiotoon]

It is now possible to notify a promise through deferred.notify() method. Notifications are useful to provide a way to send progress information to promise holders.

As there is many heavy async operation, I also see a increasingly need of reporting progress so user receive feedback. Currently, one of the best ways to cover this necessity is using callbacks. As AngularJS is based on Kris Kowal promise model, I propose the implementation of Kris Kowal's Progress Notification feature.

I've tried to follow both Kris Kowal and Angular $q philosophies. I know there is the necessity to update the DOCs, but I'd like to first discuss the implementation. As we get it done, I'll them update the docs and include in this PR.

First of, although it's a big DIFF, most of the added lines are test cases. I'd like to know from core team if the test cases covers all necessities for such a feature. Also, explaining lightly, I added the method .notify() to the deferred, based on Kris Kowal's. This method is pretty simple and do pretty much the same thing as the original: call all callbacks in the nextTick.

Some of the points, tough, I had to make a decision regarding the behavior, and these are the points I'd like to discuss. The main one are:

1. As reject, notify does not defer the progress when receiving a promise as argument, it simple notify the promise it received as is.
2. As Kris Kowal's test case, we do not save and re-emit progress notifications.
3. As Kris Kowal's, we do not notify after fulfillment or rejection.
4. In case of error while notifying, we stop notification in derived promises, although we still call all parallel callbacks in the order they were registered with the original value.
5. When using $q.when(), the listeners are attached only on the nextTick. This means that if the notification goes synchronous, we'll lose it, as we don't re-emit notification. Is this correct? Should we change promise behavior someway without braking any test?
6. When using $q.all(), we do not await for all promises to notify, instead, we defer the notification to nextTick and notify all notifications received, passing null if there was no notification for a specific promise, or omitting the property in case of hashes. Is this behavior correct?

The reason why these are my main concerns, is because this defines the feature itself, and fixing in future might not offer backwards compatibility.

Please, tell me what you think. I tried to blend in as much as possible, also trying to add the minimum amount of new code as possible. And of course, feel free to deny this feature. I would just like to know the reason, in this case.

[dbinit]

What happens when the progress callback returns a rejection? Is the promise rejected?

[caiotoon]

@dbinit as Kris Kowal's, we just notify the rejection, the promise itself is not reject.

[dbinit]

So it looks like the return value from the progress callback is ignored?

And it also looks like raising an exception doesn't reject the promise either... that's too bad. It seems like there should be a way to interrupt a long-running process this way (see my note here).

[caiotoon]

@dbinit no, return is not ignored. It's chainable.

I was thinking about your proposal, a progress listener canceling the promise. Philosophically speaking, I don't believe a progress handler could/should have enough information to decide when to interrupt a long-running process. It's a decision of someone outside the promise itself. Otherwise, we should also await for the next notification in order to cancel the long-running process.

I believe a more common scenario is where a user asks for something, then he changes his mind and asks for another. So the interruption is not in a handler, but as I said, in the place where the promise was originally handled.

Notification is only a direct channel where the deferred holder can dispatch any kind of information down to the promises.

Don't you agree?

[dbinit]

I agree. I did some more digging and found some other Defer/Promise libs that have a "canceler" option, so I've sent pull request #2452 to add that feature.

[jbdeboer]

Is this PR still active or does the $http specific #2529 satisfy your needs?

[caiotoon]

It's still active.

This PR regards adding notification feature to promises, #2529 is about canceling the $http request on $timeout, right?

[dbinit]

Yeah, this feature would be great for adding $http upload/download progress notification.

[nateabele]

@IgorMinar Any feedback on this? Adding generic progress support to $q would solve several issues, including #2725, #1934, and would make the promise API way more useful when developing & distributing 3rd-party modules.

[chirayuk]

@jeffbcross and I are taking a deep look at this and will get back soon. Thanks for this PR.

[IgorMinar]

This is an exceptional PR. Thanks a lot @caiotoon.

3 of us have reviewed this PR throughly and concluded that it's almost perfect. The only big change we made was removal of notification forwarding for $q.all. This was done because Q doesn't support this feature and it's not quite clear how to deal with several corner-cases. we might add that feature back in the future if the corner-cases are thought through.

@chirayuk made the necessary changes in #3220 (we also added/improved tests).

AFAIK #3220 is ready to be merged. Please take a look and let me know if you have any further suggestions.

[nateabele]

Woohoo! Thank you @IgorMinar and @caiotoon!

[IgorMinar]

@caiotoon here are quick answers to the questions you raised:

1. looks good to me
2. that's correct
3. that's correct
4. the error should get caught by the $exceptionHandler, I don't see that in the tests. @chirayuk can you take a look at this specific behavior and augment the test? We don't want to drop the exception.
5. that's not ideal, but since when is not a mainstream api, I'm ok with the current behavior. If this becomes a problem in practice we can address it.
6. as mentioned above we removed the notification forwarding for $q.all

[caiotoon]

Awesome!! Thanks! About the 4th point, it does log the error. Here is the test case. (the last one one)

[dbinit]

This is great. Has anyone started on adding support to $http for XHR progress events?

If not, I might have time this weekend.

[chirayuk]

Landed as 2a5c355

[caiotoon]

Thanks guys!

[IgorMinar]

Thank you @caiotoon!

I really liked this PR. Thanks for being patient with us in merging it. It's always scary for me when I see people proposing changes (especially big changes) to $q because most people don't understand the complexity of the code. (that's why this is one of the most tested pieces of our code-base). Your PR was an exception to what we commonly see.

Please fill out this form if you don't have our t-shirt yet and we'll mail you one: http://goo.gl/075Sj

[caiotoon]

Awesome, @IgorMinar!

Thank you for your words. It's good to contribute to AngularJs, it leaves a feeling of returning the favor for the so many hours you guys saved me.

Also, it's always good to be in close contact with so high quality code.

I'll try more like this one. Thanks for the t-shirt, had just filled it.

[benjamingr]

I don't want to spoil the fun, but this abstraction is doesn't really work too well in practice. Q is removing it and I think you should too. See #1934 (comment)

The alternative is simple enough - you can make the actual API call accept a progression hook.