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

Propagate progress. Fixes #113. #114

Closed
wants to merge 1 commit into from
Closed

Conversation

domenic
Copy link
Collaborator

@domenic domenic commented Sep 20, 2012

If we think this is a good idea, let's merge it; it certainly makes my stuff easier.

@kriskowal
Copy link
Owner

Alright, I’ll entertain it as an experiment. It does have the theoretical flaw that progress through a conjunction is not necessarily bound by the progress of a single operand, but you can work around that case explicitly.

Consider this case:

var A = fast();
var B = slow();
var C = Q.when(A, function () {
    return B;
});

I’m curious how that plays out. I’m guessing that you get a journey to 100%, then a drop to whatever the progress on B is at that moment, and a slow progression back up to 100%. That, or you go quickly to 100% and stall. I would have to experiment, but I’m presently working on something else.

@domenic
Copy link
Collaborator Author

domenic commented Sep 20, 2012

Oh interesting, I hadn't considered that case. Yeah we don't (and shouldn't) to any forwarding for Q.all, but even more basic cases like yours introduce some difficulties. Will test and report back.

@domenic
Copy link
Collaborator Author

domenic commented Sep 20, 2012

Here's a demo of that:

http://jsfiddle.net/cLtNS/1/

It looks like it emits progress from fast, then any progress from slow that hasn't already happened while fast was unresolved.

@domenic
Copy link
Collaborator Author

domenic commented Sep 20, 2012

Next step is to see what the other progress-using promise libraries (jQuery, When, WinJS) do.

  • jQuery emits progress from fast then all-at-once emits progress from slow that happened while fast was unresolved, then emits the rest of the progress from slow at its own pace: http://jsfiddle.net/8GkkE/1/
  • When emits progress from slow, but only those after fast resolves: http://jsfiddle.net/qe9F3/1/

@kriskowal
Copy link
Owner

Thanks for the legwork. The jsfiddle is insightful.

@ForbesLindesay
Copy link
Collaborator

This is proving to be quite a complex issue. I'd almost be inclined to require users to be explicit about what progress gets forwarded and how it gets forwarded.

@novemberborn
Copy link

This is proving to be quite a complex issue. I'd almost be inclined to require users to be explicit about what progress gets forwarded and how it gets forwarded.

I'm planning to add progress propagation to Dojo 1.8.1. If the progback returns a non-undefined value, that value is emitted as a progress update on the returned promise.

@domenic
Copy link
Collaborator Author

domenic commented Sep 23, 2012

I'm planning to add progress propagation to Dojo 1.8.1. If the progback returns a non-undefined value, that value is emitted as a progress update on the returned promise.

The problem with this approach, I would think, is that it doesn't address my first example in #113. Namely, the following works:

promiseWithProgress
.progress(console.log)
.finally(function () { })
.end();

whereas the following does not:

promiseWithProgress
.finally(function () { })
.progress(console.log)
.end();

This seems counterintuitive to me and makes using the shortcut functions (finally/fin or catch/fail) very error-prone since doing so throws away progress information.

@kriskowal
Copy link
Owner

I like the idea of translating the progress in the progback. It would also follow that progress is a single value, so we should need to lock down the arity of the progress emitter.

It is probably okay, for the purpose of progress notifications, to assume that the operation will not fail, and to reset the progress to the resolution of the handler. It is probably also okay to assume that a finally clause will not cause further delay (though this is not a guarantee).

Maybe it is okay for progress to pass-through by default, and to be able to adapt the progress in the progback. We would probably also need ways to compose progress values from other promises in the progback, perhaps analogously to how we compose promises in callbacks and errbacks.

Perhaps progress values need to be promises themselves so we can use the same composition techniques.

var A = chicagoToBoston();
var B = bostonToChicago();
var C = Q.progress(A, function (aProgress) {
    return Q.progress(B, function (bProgress) {
        return // ?
    });
});

@novemberborn
Copy link

It would also follow that progress is a single value, so we should need to lock down the arity of the progress emitter.

IIRC deferreds only have single resolution, rejection or progress values.

It is probably okay, for the purpose of progress notifications, to assume that the operation will not fail, and to reset the progress to the resolution of the handler.

Does that assumption mean there is no rejection of the promise if the operation does fail? I don't think that's right.

Maybe it is okay for progress to pass-through by default, and to be able to adapt the progress in the progback.

In Dojo the progress is pass-through if there is no progback registered.

@domenic
Copy link
Collaborator Author

domenic commented Sep 25, 2012

IIRC deferreds only have single resolution, rejection or progress values.

If you recall what correctly? Promises/A gives no guidance. On the other hand all libraries but jQuery do have single progress values (and jQuery doesn't count, they don't even have single fulfillment values or rejection reasons). More discussion on Q Continuum.

In Dojo the progress is pass-through if there is no progback registered.

This makes sense, but I'm not sure I gather the exact semantics of the transformation. How does this behave?

promise.then(null, null, function (x) {
    return Q.delay(1000).then(function () { return x + 10; });
})
.then(f, r, p);

Does p receive a promise, or the eventual value? In particular can p be called after f/r?

@novemberborn
Copy link

If you recall what correctly? Promises/A gives no guidance. On the other hand all libraries but jQuery do have single progress values (and jQuery doesn't count, they don't even have single fulfillment values or rejection reasons).

Ha, fair enough. Single values all the way for me.

I'm not familiar with the promise.progress() syntax you use, so allow me to give an example:

var dfd = new Deferred();
dfd.then(f, r, p);

dfd.progress("foo"); // --> p("foo")
dfd.progress(otherPromise); // --> p(otherPromise)
dfd.resolve("bar");  // --> f("bar");
dfd.progress("baz"); // noop

@domenic
Copy link
Collaborator Author

domenic commented Sep 26, 2012

I'm not familiar with the promise.progress() syntax you use, so allow me to give an example:

Sorry, promise.progress(p) in Q is just sugar for promise.then(null, null, p). I'll edit for clarity. The example I gave was trying to figure out how returning a promise from a progressback behaves. Any feedback on that?

I agree that notifying a deferred of progress (deferred.notify() in Q) after it's already been resolved should be a noop.

@novemberborn
Copy link

The example I gave was trying to figure out how returning a promise from a progressback behaves. Any feedback on that?

The promise is passed on as a progress update.

I don't think progress is used often enough to warrant much sugar. In Dojo for instance there is no sugar for promise.then(null, null, p), yet there is promise.otherwise(e) for promise.then(null, e). It's just not a common use case.

For the scenario you're describing:

var otherPromise = promise.progress(function(update){
  // Process the update asynchronously, return a promise to propagate more progress later…
  return asyncPromise;
});

Without magic handling of the progress promise you can still achieve the same by manually forwarding the promise state:

var otherDeferred = new Deferred(promise.cancel);
promise.then(otherDeferred.resolve, otherDeferred.reject, function(update){
  asyncPromise.then(null, null, otherDeferred.progress);
});

var otherPromise = otherDeferred.promise;

@domenic
Copy link
Collaborator Author

domenic commented Oct 13, 2012

Superceded by #125.

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

Successfully merging this pull request may close these issues.

None yet

4 participants