Skip to content

Loading…

Rocketpack changes to upstream #94

Closed
wants to merge 2 commits into from

3 participants

@sakari

Hi

we've been using when for a while now and made some light additions to it. Would you be interested in merging some of this to cujojs/when or should we spin of a separate package that would provide these using when?

The tests pass, but I seem to be unable to run lint successfully even in your master.

Jaakko Manninen and others added some commits
Jaakko Manninen f(), partial(), and public fulfilled() 610e540
Sakari Jokinen map with 'throttle'
A call to throttle the map to limit the resource usage like number of
open files.
e886980
@renato-zannon

Hey @sakari! Your changes seem to have some overlap with when/function (see #80), that are coming out on 1.8.0. Please check if I'm not mistaken!

The exception seems to be your throttle function. If I understood correctly, it is made to execute multiple tasks concurrently, but with a cap on how many can run simultaneously, is that correct? If it is, what do you think about adding the max parameter to when.map itself?

About turning fulfilled() public, it seems to me that when.resolve already covers that :)

The tests pass, but I seem to be unable to run lint successfully even in your master.

Have you been able to run the lint? It seems that travis has detected a function with complexity higher than the threshold on when's .jshintrc. Fixing it should be as easy as breaking the function in more pieces :)

@sakari

re: merging map and throttle, I'm personally opposed to adding flag arguments to calls, but could do it if that's what it takes. Something like when.map(max, arr, fn) with possibility to call when.map(arr, fn) to get the unthrottled behaviour?

@renato-zannon

I was thinking more on the lines of when.map(arr, fn, max), with an optional max parameter. But looking back at the directory structure (and files like parallel.js and sequence.js, that are similar to your proposal), maybe it would be better to add a separate file for that, to make it clearer that it is opt-in behavior. What do you think?

@briancavalier
The Javascript Architectural Toolkit member

Hi @sakari, thanks for putting this pull request together! As @riccieri mentioned, there is overlap here with the when/function and when/node/function modules that are coming in 1.8.0. However, I actually see this as a good thing, since it reinforces the fact that this kind of functionality will be useful to people :)

Please take a look at the dev branch and see if those two modules cover your use cases. If they do, then I think we're good, but if not, please let us know and we can discuss how to handle the situation.

Exposing fulfilled is not something we can do for a few reasons: it breaks the identity function, and it creates refactoring hazards near the beginning of a promise chain. As @riccieri mentions, when.resolve() is the preferred public API for creating already-fulfilled promises.

I like the idea of throttle very much, and would love to incorporate the functionality. I can see it being applicable to both when.map and when/parallel. The key, as you guys have been discussing, is finding the right API. In general, we favor separate modules over adding more methods onto when (the fact that many methods, such as when.map exist, is mostly a throwback to the fact that I started when.js as a single file).

Here are a couple initial thoughts to keep the discussion moving.

  1. when.map.throttle(), when/parallel.throttle()
  2. when.map(array, mapFunc, [maxConcurrency]). Unfortunately, when/parallel is problematic with this approach, as it already allows additional varargs. Not sure how we could provide a similar style. Any ideas?
  3. when.mapThrottle(), when/parallelThrottle()
  4. when/map/throttle(), when/parallel/throttle()

Thoughts? Once we agree on an overall API approach, we can dig into the code itself. Thanks!

@sakari

One approach would be to provide a deferred aware semaphore for limiting how many deferreds are running at a time and sidestep the question altogether:

when.map(arr, when.semaphore(10).do(function(...) {
    ...only 10 at a time get here...
    return a deferred;
}));

Do you have a anything for doing partial application in the works? in ours when.partial is handy for

.then(when.partial(binaryFunction, arg1))

Which is very common. Although we could use https://github.com/rook2pawn/node-partial for that.

When are you planning to release 1.8?

@briancavalier
The Javascript Architectural Toolkit member

Your semaphore idea sounds very interesting! I love the idea of finding a way to do generalized concurrency limiting. Would you be willing to do a proof of concept implementation to help us visualize how it would work?

I've been contemplating a method named when.each(), which sounds like it could be similar in some ways, but allowing full parallelization. So maybe when.semaphone and when.each would eventually be able to share some code.

Have a look at bind() in when/function. It does promise-aware partial application, and will even allow promises as partially applied arguments. I think it will fit your needs, and would work as expected in the example snippet you provided. I agree this is a very nice use of partial application!

@sakari sakari referenced this pull request
Closed

basic `guard` functionality #96

@sakari

I think our needs are covered by #96 and stuff brewing in master so I'm closing this.

@sakari sakari closed this
@sakari sakari deleted the rocketpack:rocketpack-changes-to-upstream branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 30, 2012
  1. f(), partial(), and public fulfilled()

    Jaakko Manninen committed
Commits on Feb 7, 2013
  1. map with 'throttle'

    Sakari Jokinen committed
    A call to throttle the map to limit the resource usage like number of
    open files.
Showing with 133 additions and 1 deletion.
  1. +58 −0 test/throttle.js
  2. +75 −1 when.js
View
58 test/throttle.js
@@ -0,0 +1,58 @@
+(function(buster, when) {
+
+var assert, refute, fail;
+
+assert = buster.assert;
+refute = buster.refute;
+fail = buster.assertions.fail;
+
+buster.testCase('when.throttle', {
+ 'should throttle calling the map function': function(done) {
+ var dfd = when.defer();
+ when.throttle(1, [1, 2], function(v) {
+ assert.equals(v, 1);
+ return dfd;
+ });
+ done();
+ },
+ 'should map the array': function(done) {
+ when.throttle(2, [1, 2], function(v) {
+ return when.defer().resolve(v * 10);
+ }).then(function(v) {
+ assert.equals(v, [10, 20]);
+ done();
+ });
+ },
+ 'should call map function when there is space': function(done) {
+ var dfds = [when.defer(), when.defer()];
+ var called = {};
+ when.throttle(1, [0, 1], function(v) {
+ called[v] = true;
+ return dfds[v];
+ });
+ assert.equals(called, { 0: true });
+ dfds[0].resolve('a');
+ assert.equals(called, { 0: true, 1: true});
+ done();
+ },
+ 'should resolve only after all tasks are resolved': function(done) {
+ var dfds = [when.defer(), when.defer()];
+ var resolved = false;
+ when.throttle(10, [0, 1], function(v) {
+ return dfds[v];
+ }).then(function() {
+ resolved = true;
+ });
+
+ dfds[0].resolve();
+ assert.equals(resolved, false);
+ dfds[1].resolve();
+ assert.equals(resolved, true);
+ done();
+ }
+});
+})(
+ this.buster || require('buster'),
+ this.when || require('../when')
+);
+
View
76 when.js
@@ -21,6 +21,7 @@ define(['module'], function () {
when.defer = defer; // Create a deferred
when.resolve = resolve; // Create a resolved promise
when.reject = reject; // Create a rejected promise
+ when.fulfilled = fulfilled;
when.join = join; // Join 2 or more promises
@@ -30,11 +31,85 @@ define(['module'], function () {
when.map = map; // Array.map() for promises
when.reduce = reduce; // Array.reduce() for promises
+ when.throttle = throttle; // Array.map() with throttling
when.chain = chain; // Make a promise trigger another resolver
when.isPromise = isPromise; // Determine if a thing is a promise
+ when.f = f;
+ when.partial = partial;
+
+ function throttle(max, arr, fn) {
+ var running = 0;
+ var next = 0;
+ var results = [];
+ var dfd = when.defer();
+
+ function runNext() {
+ if(!dfd) {
+ return;
+ }
+ if(next >= arr.length) {
+ if(running === 0) {
+ return dfd.resolve(results);
+ }
+ return;
+ }
+ if(running >= max) {
+ return;
+ }
+ running++;
+ var ix = next++;
+ fn(arr[ix]).then(function(r) {
+ running--;
+ results[ix] = r;
+ runNext();
+ }).otherwise(function(e) {
+ dfd.reject(e);
+ dfd = null;
+ });
+ }
+
+ arr.forEach(function() { runNext(); });
+ return dfd;
+ }
+
+ /** wrap given function (node) into a promise and call it with arguments */
+ function f(node) {
+ var args = [];
+ for(var i = 1; i < arguments.length; i++) {
+ args.push(arguments[i]);
+ }
+ var dfd = when.defer();
+ args.push(function(err) {
+ var args = [];
+ if(err) {
+ return dfd.reject.apply(null, arguments);
+ }
+ for(var i = 1; i < arguments.length; i++) {
+ args.push(arguments[i]);
+ }
+ return dfd.resolve.apply(null, args);
+ });
+ node.apply(null, args);
+ return dfd;
+ }
+
+ /** partially apply function to arguments */
+ function partial(fn) {
+ var args = [];
+ for(var i = 1; i < arguments.length; i++) {
+ args.push(arguments[i]);
+ }
+ return function() {
+ for (var i = 0; i < arguments.length; i++) {
+ args.push(arguments[i]);
+ }
+ return fn.apply(null, args);
+ };
+ }
+
/**
* Register an observer for a promise or immediate value.
* @function
@@ -169,7 +244,6 @@ define(['module'], function () {
/**
* Create an already-resolved promise for the supplied value
- * @private
*
* @param value anything
* @return {Promise}
Something went wrong with that request. Please try again.