cujojs / when

A solid, fast Promises/A+ and when() implementation, plus other async goodies.
Other
3.44k stars 396 forks source link

Method for registering *only* a progress handler #104

Closed briancavalier closed 10 years ago

briancavalier commented 11 years ago

Since we have otherwise and always as convenience forms of then, it may make sense to provide a convenience method for registering only a progress handler.

There are some open questions here:

  1. What is it named?
  2. What does it return? It'd use then internally, but returning the resulting new promise may be pointless or confusing, or maybe not returning the resulting promise is confusing?

Some name options:

Twisol commented 11 years ago

Actually, I'm surprised there isn't one already! It seems almost a necessity if you don't want any undefined, undefined, cruft.

The problem with choosing a name is that the name should play well with the success- and failure-path function names. That's hard, because temporally they happen at different times. Progress events essentially add a second reified execution timeline to a promise! I actually think progress events don't belong in the Promise, but rather a similar Signal that doesn't freeze after resolution. But, that's neither here nor there.

Responding to your points:

  1. Not a fan of the first two for the reasons you mentioned. The rest, when combined with existing functions, suggest execution in the same timeline as then, otherwise, etc., which will be pretty confusing. Perhaps something relating to the "incompleteness" of data. Progress events are progressing towards some final result, similar to how onreadystatechange works.
  2. SInce the progress path is a chain of handlers just like the success and failure paths, I think it would be confusing if it didn't return a promise. I think it would be best if it were just a shorthand for .then(undef, undef, onProgress).

Names: loading... that's all I can think of, sorry.

briancavalier commented 11 years ago

That's hard, because temporally they happen at different times.

You've basically hit the nail on the head as to why I generally dislike progress events, and specifically the fact they were conflated into then. The onFulfilled and onRejected handlers deal with the time after a promise has been settled (fulfilled or rejected), but the onProgress handler deals with the time before that. That is, they deal with completely disjoint times in a promise's lifecycle, yet they have been smashed into the signature of a single method named then.

Part of me wishes I had never added them in the first place. As you said, tho, neither here nor there :) We have them, people are using them and finding them useful.

The rest, when combined with existing functions, suggest execution in the same timeline as then, otherwise, etc., which will be pretty confusing

Agree, stuff like this is confusing:

// Using onProgress for clarity, not because I like it :)
var p2 = p1.onProgress(handleProgress1);
p2.onProgress(handleProgress2);

handleProgress1 handles progress toward p1, not from p1 to p2 as some might interpret it. handleProgress2 can handle progress toward both p1 and p2, due to progress propagation. Confusion usually ensues.

I think it would be best if it were just a shorthand for .then(undef, undef, onProgress)

Yep, I'm thinking it would be exactly this since we're stuck with progress handling being smashed into then.

Twisol commented 11 years ago

handleProgress1 handles progress toward p1, not from p1 to p2 as some might interpret it. handleProgress2 can handle progress toward both p1 and p2, due to progress propagation. Confusion usually ensues.

Oh, crud. I didn't even think of that particular wrinkle. Umm...

I don't suppose factoring progress events into their own conceptual object (i.e. a signal) would be something you'd consider for a breaking release? You could bundle a signal with each promise if you still wanted all three types of events available, but it would also prevent the problematic interleaving of timelines:

var deferred = defer();
deferred.signal.then(useIntermediate1).otherwise(fixIntermediate2);
deferred.promise.then(handleFinal1).then(handleFinal2);

I'd rather make a clean break, myself - the user can create a signal if they want one - but either way, it does separate the timelines. (And it lets you propagate errors properly in the progress timeline, something that isn't possible now.)

briancavalier commented 11 years ago

I would love to do a clean break. It's something we should consider. It will have to be a major release, and can't be 2.0, so would need to be 3.0.

We can also bide our time a bit as Promises/A+ wrestles with the issues around progress, too. We may yet come up with something workable within the confines of then, but I have my doubts.

Twisol commented 11 years ago

Sounds good. I think Promises are like the gateway drug to Functional Reactive Programming, so moving in that direction can only be beneficial. For now, the only way to avoid confusion is to not intermix the intermediate and resolved timelines.

What if resolver.progress was renamed to resolver.update, and we reclaimed progress for our purposes here?

briancavalier commented 11 years ago

I like resolver.update. Let's deprecate resolver.progress in 1.8.1, and remove in 2.0. Probably means we wait for 2.0 before introducing promise.progress to avoid confusing people, and I'm ok with that.

briancavalier commented 11 years ago

Paved the way for this in 041dd2b1e7c990a23daf28ebc6c1d2d4cc44e66b

briancavalier commented 11 years ago

Now that we removed deferred[.resovlver].progress in 2.0. We could probably introduce promise.progress(onProgress) as a way to register only a progress handler. But, I'd like to see if anyone has other clever naming ideas for this.

@jaredcacurak any ideas?

clearly commented 11 years ago

I'm fine with resolver.update...to be 100% clear: 2.5.1 does NOT have a way to register only a progress handler? True or False? The thread was hard to follow on this point.

Twisol commented 11 years ago

@clearly: There is a way: .then(undefined, undefined, progressHandler). What is/was under discussion is whether a convenience method should be added so you don't have to worry about passing undefined for the other two handlers.

briancavalier commented 11 years ago

Hey, @clearly. A while back, we settled on resolver.notify (rather than resolver.update) as the method for issuing progress events. See #106. And yep, you are correct: As of 2.5.1, there is no convenience method for adding only a progress handler, and using then as @Twisol shows is the only way.

Since resolver.progress has been gone for a while, we do have the option of reclaiming the name for this new "register only a progress handler" shortcut: promise.progress(onProgress) ~= promise.then(undefined, undefined, onProgress)

clearly commented 11 years ago

Yes I understand that the third handler can be given to a .then but the convenience methods make a huge difference in readability.

resolver.progress +1

briancavalier commented 11 years ago

Ok, I'm moderately in favor of promise.progress(onProgress):Promise as the API for registering only a progress handler. It would be implemented exactly as this: return promise.then(undef, undef, onProgress);

Things I like: It's an obvious name, isn't camelCase, and resolver.progress has been gone long enough to avoid naming collisions.

Things I don't like: "progress" is a noun or a verb. Since it's a method, reading it as a noun makes little sense. As a verb, proGRESS, it implies "to make progress" rather that what the method actually does which is "to receive progress notifications". So the name is a little weird.

Anyone have any better suggestions for a name? I'd like to get this in soon.

webpro commented 11 years ago

How about just promise.onProgress? The on part clearly states events/notifications. It's camelCase, but imho it makes more sense than just progress, as you explain.

briancavalier commented 11 years ago

Hmmm, yeah, onProgress is more clear. Ok, I'm slightly favoring onProgress over progress now, despite the camelCase :) Thanks for the suggestion, @webpro.

What do others think?

clearly commented 11 years ago

I think one of the most important issues for naming public API methods is consistency. There isn't necessarily anything wrong with .onProgress but if I were a new user I would be confused if/why there were or were not an .onOtherwise or .onThen which are the other forms of convenience methods(first param of .then of course). I'm in favor of just .progress as I think it is more consistent.

briancavalier commented 11 years ago

I hear you, @clearly. I agree that consistency definitely important. The thing that is swaying me here is that progress is a very different thing than basically every other promise method. Even though Promises/A (not A+) originally smashed it (optionally) into then, it really does not belong there.

As a simple example of why: progress deals with things that only happen before the promise's resolution, and every other method (including then if you consider only the onFulfilled and onRejected arguments) deals with things that only happen after resolution.

There are other reasons progress is the oddball. So, yes, I would love to find a consistent name. However, I'm not quite as bothered by the progress API not looking like the others since it isn't really like them in functionality.

briancavalier commented 11 years ago

progressed and progressing are not bad.

I also just noticed that Q uses the name progress, and bluebird uses progressed. So, in either case, a bit of consistency there would be a benefit for cross-lib learnability.

briancavalier commented 10 years ago

This is in the when3 branch as promise.progress(onProgress).