cujojs / when

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

Task context in sequence, pipeline, parallel #79

Closed briancavalier closed 11 years ago

briancavalier commented 11 years ago

Personally, I'd be ok with relying on bind() in all cases, but some folks might prefer if there were an easy way to supply a context for tasks.

Right now, tasks are called like task.apply(null, ...), or directly like task(arg). Maybe we should be capturing the context upon entering sequence/pipeline/parallel and passing that along to each task?

Beyond that, of someone wants more fine grained control than that, they can bind() the tasks a priori.

renato-zannon commented 11 years ago

I think there's a valid point on preferring task.apply(myContext, /*...*/) to using Function.prototype.bind: calling the function on the context instead of creating a bound function is currently much faster on all browsers.

I think that capturing the context on which the functions are called is a good strategy, since the functions are variadic and we can't just use an optional last parameter.

briancavalier commented 11 years ago

Good points. And to add about the context parameter: Using the 2nd parameter is pretty ugly, too, imho: sequence(tasks, context, ...) because it leads to using null or undefined all over the place when you don't want to supply a context: sequence(tasks, null, ...) blech.

I'm sure bind() performance will eventually catch up once enough people start complaining about how slow it is :) I do think that most of the time, the time to perform an async task (xhr, filesystem access, database access, etc.) will completely dominate the bind() overhead.

There are times where capturing the context won't be enough, of course: If someone wants, for example, each task to have a different context. The caller will have to handle that with bind() or closures, which seems reasonable.

renato-zannon commented 11 years ago

Good point about the overhead being dominated by the asynchronous task.

Have you seen any real use case for this? While I can see its usefulness, having some use case to optimize towards might help seeing other solutions. Also, is it an option to add new, context-accepting versions of sequence/pipeline/parallel, to maintain backwards-compatibility with the current versions?

briancavalier commented 11 years ago

TBH, I haven't seen any concrete use cases for the context yet. We use sequence and pipeline in wire.js, but don't need a context currently. So yeah, I think waiting for some real uses cases and using this issue as a place for when.js users to discuss is the right thing for now.

While I'm not personally a fan of the "one variation that accepts a context and one that doesn't" approach, if that turned out to be the most convenient, then yep, we'd need to consider it.

briancavalier commented 11 years ago

I haven't heard any needs for this, so I'm removing the milestone until we hear from more folks who want/need it.

mikaelkaron commented 11 years ago

Actually, I'd need (or would use) that.

At the moment I'm controlling context outside of when.js, but it would be nice if it was supported out of the box.

Would it be possible to reuse the context that when was originally executed with? So if I did when.call(context, ...) that would set the context (probably not, but it's an idea).

briancavalier commented 11 years ago

Hey @mikaelkaron,

Are you using when/sequence, when/pipeline, and when/parallel? I couldn't tell from your message. This issue is specifically directed at those, since they take an array of functions as their input. That makes it extra annoying to have to bind the context for every function in the array. If you are using them, would being able to specify a single context to use with all the input functions be useful to you?

As for single handlers registered with when or .then, it will most likely be disallowed by Promises/A+ to ensure consistency across promise implementations--my current feeling is that .bind() is the right solution here. We can still allow it for sequence/pipeline/parallel, of course.

mikaelkaron commented 11 years ago

I use when/pipeline in places. But maybe adding this would mess with the symmetry of when.js. I think I'll retract the "I'd like this" and say "I want it to work the same way as then"

briancavalier commented 11 years ago

I want it to work the same way as then

Yeah, this seems like the right way to go.

If someone presents a valid use case for specifying the same context across all the functions in a sequence/pipeline/parallel array, then we can revisit. So, I'm content with not making any changes for now, since we don't have any concrete use cases for it. Closing.