Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Task context in sequence, pipeline, parallel #79

Closed
briancavalier opened this Issue Nov 28, 2012 · 9 comments

Comments

Projects
None yet
3 participants
Owner

briancavalier commented Nov 28, 2012

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.

Contributor

renato-zannon commented Dec 3, 2012

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.

Owner

briancavalier commented Dec 3, 2012

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.

Contributor

renato-zannon commented Dec 3, 2012

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?

Owner

briancavalier commented Dec 3, 2012

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.

Owner

briancavalier commented Jan 16, 2013

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

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).

Owner

briancavalier commented Jan 28, 2013

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.

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"

Owner

briancavalier commented Feb 14, 2013

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.

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