cujojs / when

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

Deprecate progress #371

Closed briancavalier closed 9 years ago

briancavalier commented 10 years ago

As discussed many times over, there's no standard for promise progress, libraries implement it differently, and there are problems with those that do (when.js included). I'm voting to drop the current notion of progress entirely, in favor of other things that likely solve the problem in a better way:

  1. Streams via most.js. They can already propagate a sequence of asynchronous events and end with a final result (a promise for it, in fact), which matches many progress use cases. They can deal with the case of wanting to compute progress asynchronously (ie "Why can't I return a promise from a promise progress handler??"), and they have nice filtering, transforming, reducing etc. operations.
  2. See discussion about ETA (and other potential alternatives) in #264. I'm not convinced this is needed, but it is interesting, fairly easy to compute, and corresponds more directly to the notion of a promise as a placeholder for a result (ie a future value) and not the operation which will produce the result (ie the "task").
rkaw92 commented 10 years ago

I'm all for it.

Progress updates could be solved, for example, by tapping into promises and generating side effects (for example, emit()ting events from an ad-hoc EventEmitter). Functions which are supposed to make progress and report it could accept those emitter objects in their parameters, thus avoiding a dependency on any particular emitter constructor API in long-running task sequences.

Emitter composition (cascading) is also not very hard, considering that one could pass the same emitter several levels deep or use a bridge callback that calls the other emitter's emit().

One pitfall is that the event names will not be known immediately, so it might be best to use an emission function instead of passing in a whole emitter object. That way, all progress updates behave the same way - there is no distinction between various "types" and no guessing as to event names to use to listen to the updates.

Note how the below "pitfall" can be enabled or disabled per-application, depending on individual needs. Some will require precise event type recognition and division into I/O operations, deferred computations, etc., while others will be happy with just reporting that something is happening.

As a bonus, the distinction between eventual results and long-running processes becomes a programmer concern and is out of when.js' scope if progress updates are phased out. This is really a case of notifying the caller when something is going to be completed vs. telling them what is happening as it progresses. Both styles must be available to the programmer and it could prove vain to try and include them in when. (The question is: is one a derivative of the other? It very well could be, if you take a FSM and grant it the knowledge of the event types that a process is composed of - then it could step through the states, each representing a percentage of completion. This is leaping forward a bit too much, though, and the cracks are clearly visible...)

This is, of course, one of the many options. In general, I do not think that when.js as such should provide any facility for progress reporting in long-running processes until a standard emerges. This should be handled by a different library.

scothis commented 10 years ago

+1 progress events are just reliable enough to be dangerous.

On Mon, Sep 15, 2014 at 4:47 PM, Brian Cavalier notifications@github.com wrote:

As discussed many times over, there's no standard for promise progress, libraries implement it differently, and there are problems with those that do (when.js included). I'm voting to drop the current notion of progress entirely, in favor of other things that likely solve the problem in a better way:

1.

Streams via most.js https://github.com/cujojs/most. They can already propagate a sequence of asynchronous events and end with a final result (a promise for it, in fact), which matches many progress use cases. They can deal with the case of wanting to compute progress asynchronously (ie "Why can't I return a promise from a promise progress handler??"), and they have nice filtering, transforming, reducing etc. operations. 2.

See discussion about ETA (and other potential alternatives) in #264 https://github.com/cujojs/when/issues/264. I'm not convinced this is needed, but it is interesting, fairly easy to compute, and corresponds more directly to the notion of a promise as a placeholder for a result (ie a future value) and not the operation which will produce the result (ie the "task").

— Reply to this email directly or view it on GitHub https://github.com/cujojs/when/issues/371.

briancavalier commented 10 years ago

@rkaw92 Exactly, there are other, better ways to do incremental computing, and a promise represents a result, not a process.

@scothis Yep

Here's a simple strawman plan:

  1. Deprecate progress in 3.5.0
    • Add @deprecated to jsdoc and point it out in documentation.
    • Add examples of how to do some simple "progress-like" things using streams.
  2. Remove entirely in 4.0.0
    • In an "upgrading from 3.x to 4.0" section of the docs, we can state that it was removed and point to the afore-mentioned stream examples.
    • Code attempting to issue progress notifications should fail spectacularly (eg undefined is not a function or somesuch)
    • This also means we'll become a black hole for progress events when iteroperating with other promise implementations that still support it. I'm ok with that, since that's probably only working by luck right now anyway.
jescalan commented 10 years ago

I must admit I am very sad about this as I use progress events for a lot of things right now, but I suppose as long as most.js is done by then, this will be a better way to handle these situations?

briancavalier commented 10 years ago

as long as most.js is done by then, this will be a better way to handle these situations?

Way better :) most 0.7 is quite usable, but still needs docs. It seems like a good goal would be to have the most.js docs in a usable state by the time when 3.5.0 lands and deprecates progress.

I use progress events for a lot of things right now

One thing I'd love to do is show examples of how to use streams to solve the same problems for which people are currently using progress. It'd be great to talk sometime about some of your use cases ... are there any in public code that you can link to? I could even take a shot at showing how streams might solve the same problems.

briancavalier commented 10 years ago

See also #272

jescalan commented 10 years ago

Here's a quick example:

https://github.com/jenius/roots/blob/v3/lib/api/new.coffee#L36

When creating a new project using roots, there's a lot that has to be done. Fetching the template, making sure it's up to date, creating the folder and files, running npm install, etc. While the API endpoint returns a promise, it emits progress events along the way to notify the user about what's going on. In addition, the CLI uses these to display output if you are running it from the command line. I feel like this is a perfect use-case for promise progress.

At the same time I'm more than happy to start converting over to streams because there are a few other use cases where it was more of an awkward fit.

briancavalier commented 10 years ago

@jenius Cool, thanks!

In this particular case, I wouldn't use a stream. The reason is that the code is primarily using promises to sequence side-effectual async tasks, rather than to compute/transform a value. The "extra" deferred is really just being used as an event emitter.

I think I'd probably just pass in a progress callback, either directly as a second param, or as one of the properties of the opts object. I took a shot at it in this gist. I used ES6 rather than coffeescript, but it's still quite similar to the original code :)

I like the fact that there is only one promise chain, rather than two (ie no need for the extra deferred). It's also slightly interesting in that using .tap() would allow the notify function to return promises and participate in the promise chain if you needed that (except for the first usage of notify in install_deps where it wouldn't work as is).

Any thoughts?

jescalan commented 10 years ago

Wow, this is a lot of great feedback here! Thanks so much for taking the time to do this, I'm sure it was not trivial, and is much appreciated. Seems like a great way to do things, I might switch it over! This framework is becoming really huge and any architecture changes that help to wrangle it are very useful.

briancavalier commented 10 years ago

@jenius Cool, glad that was helpful. That tap pattern may end up being one of several that people can use to replace promise progress.

If you run into other spots where it's not clear how to replace progress, feel free to ping me on IRC, and we can work through them.

jescalan commented 9 years ago

Thanks so much, might take advantage of that :grinning:

briancavalier commented 9 years ago

Progress was deprecated in #372, which was released in 3.5.0. Closing.