Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Rewrite pop() to be non-recursive? #4

Closed
ZJONSSON opened this Issue · 3 comments

2 participants

@ZJONSSON

I'm a "long-time" user of Queue and think it's a very elegant framework to manage control over multiple callbacks.
I did two modifications which I wanted to share. I'm not sure of the pros and cons, but they seem to work.

The first one allows me to specify the "await" before any "defer" without risking premature zero results.

Line 33:

if (!remaining) await(error, results);
->
setTimeout(function() { if (!remaining) await(error, results) },0)

And the second change is to circumvent any potential Maximum Recursions by putting a similar timeout on the pop() in defer:
Line 26:
pop()
->
setTimeout(pop,0);

@mbostock
Owner

Seems like a good idea to rewrite pop() to not be recursive, but I don’t have the spare brain power to think through at the moment how that would be done. I’d rather not resort to setTimeout(pop, 0) because that is quite expensive. On Node.js, it’d be easy to switch to process.nextTick, but I’d like Queue to function consistently in the browser.

Also, I think it’s a feature (although a debatable one) that queue.await invokes immediately if all deferred tasks have completed. In theory you could reuse the same queue multiple times, although I'm not sure how well that is supported at the moment. Related: I way of canceling deferred tasks that have not started and inserting deferred tasks at the head of the queue rather than the tail would be helpful.

@ZJONSSON

Agreed. There is a hack (using windows.postmessage) that delivers a more efficient solution but I'm unsure if it's fit to be a part of a standard library or not: http://jsperf.com/nexttick-vs-setzerotimeout-vs-settimeout/ An efficient processNext() in a browser would nevertheless open up a whole new range of interesting code patterns, i.e. "execute this, when the chain is done" type of logic.

@ZJONSSON ZJONSSON closed this
@mbostock
Owner

This is fixed in 7bd5059.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.