New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Promise.series #134

Closed
epoberezkin opened this Issue Mar 7, 2014 · 21 comments

Comments

Projects
None yet
10 participants
@epoberezkin

epoberezkin commented Mar 7, 2014

When you have the list of values to be passed to the same function sequentially, so that the next call is only made only when the promise returned by the previous call is resolved, you either have to "manually" chain promises or use Promise.reduce, which both looks a bit ugly and also not so obvious.

It could be nice to have something like this:

Promise.series = function (arr, iteratorAsync) {
    return Promise.reduce(arr, function(memo, item) {
        return iteratorAsync(item);
    }, 0);
}
@benjamingr

This comment has been minimized.

Collaborator

benjamingr commented Mar 7, 2014

How is this any different from .reduce apart from the arbitrary 0 default value?

Also, duplicate of #109 and #70

@epoberezkin

This comment has been minimized.

epoberezkin commented Mar 7, 2014

Sorry. Correct. Maybe it's still needed this way or another as I'm the third to suggest it :)
It's probably that control-flow mentality coming from callbacks.
The difference is in using 1 line of code:

Promise.series(arr, iteratorAsync);

instead of 3 lines (or 1 long not too readable line):

Promise.reduce(arr, function(memo, item) {
    return iteratorAsync(item);
}, 0);

And in case when the results are collected in array as somebody suggested (mapSeries) it becomes even bigger difference.

@epoberezkin epoberezkin closed this Mar 7, 2014

@petkaantonov

This comment has been minimized.

Owner

petkaantonov commented Mar 7, 2014

The problem is this:

You are pulling iteratorAsync out of nowhere but not doing the same for reduce case.

A fair comparison would be:

Promise.reduce(arr, reducerAsync, 0);

So you have not gained anything because instead of making iteratorAsync function you will just make reducerAsync function. And there is no difference with array case at all - you just pass array as initial value instead of 0.

@petkaantonov

This comment has been minimized.

Owner

petkaantonov commented Mar 7, 2014

The reason for popularity of this suggestion is misunderstanding of promises and thinking of Promise.all as some kind of "parallel" which needs a sequential equivalent.

But Promise.all doesn't "parallelize" promises or anything, it does nothing to the input promises - it is simply just telling you whether the promises you already are controlling from somewhere else are completed or not.

So because promises are not being controlled from the collection methods, Promise.all cannot have a .sequence equivalent - a Promise.sequence would always need a tailored "iteratorAsync" function to go with it - just like reduce so no net savings of lines of code are gained .

@epoberezkin

This comment has been minimized.

epoberezkin commented Mar 7, 2014

I agree in general that desire to have serial equivalent comes from callbacks world.
But there is an important difference between reduce and "Promise.seq", "mapSeq" etc. iteratorAsync does not come form nowhere - it is usually a function that already exists (or can be made with promisify) and does something useful; it often already takes one parameter and returns a promise (and even if it takes more than one parameter it can be transformed with partial application) .
On the other hand, the function for reduce is almost always specially constructed, as its signature is defined by reduce and dictated by the task of collecting multiple values into one.
And while reduce can be adapted for a different task, namely "chaining" promises generated by the same function using array of parameters, it feels unnatural to use it for such purpose and will always cause additional anonymous function that is never defined.
But it is ok. Case closed.
If you reconsider, I'm happy to implement. Seq's (or whatever you would call it) presence would definitely simplify adoption of promises by callback-minded people who are unfortunately still a majority :).

@spion

This comment has been minimized.

Collaborator

spion commented Mar 8, 2014

@epoberezkin, I agree with all you said, but I think there is a tradeoff there.

Before switching to promises, I used to use callbacks. At a certain point, I realized that there is no equivalent of mapSeries provided by most promise libraries, and I asked for it.

However, when I saw a simple implementation of mapSeries for the first time, I had a revelation. Oh wow, with promises, mapSeries is just a map over an array with an intermediate variable that makes sure operations are ordered correctly!

It really helped me grasp the power of promises. What I realized then is that their application is much broader than just simple arrays, and that having unfinished computations as values, (if done right) can provide endless flexibility in combining them.

If the library I picked then (Q) had a mapSeries eqivalent, I'm not sure that any of this would've happened. I don't think I would've seen the point of promises at all. I'd gloss over it, see the same old functions as in caolan's async, no improvements - why would I even use these when I can use caolan's async without any wrapping at all?

@epoberezkin

This comment has been minimized.

epoberezkin commented Mar 9, 2014

Interesting. Usually it's the only way revelations happen - because of the lack of something :)

@ericclemmons

This comment has been minimized.

ericclemmons commented Mar 10, 2014

Thank God I found this issue. I was under the impression that Promise.all was iterative. (I was .push'ing promises onto an Array, expecting sequential execution).

@yourcelf

This comment has been minimized.

Contributor

yourcelf commented Nov 13, 2014

It's not just "callback-minded people" that need mapSeries-like operation. Sometimes the things you're doing just need to be run serially.

A real world example I've been working on today: a unit test that runs a promise chain a bunch of times, with different parameters each time. Something like:

for each parameterSet in testcases:
    run promise chain with parameterSet
done

Since a single shared state (a live server, database, etc) was being altered by each iteration of the promise chain, the tests couldn't run in parallel, or they'd trip each other up.

I'm happy with Promise.reduce as a way to accomplish what I wanted. But it's not a semantically accurate for what I'm doing -- someone reading my code will understand it less well, because I'm using reduce for the side effect of sequential iteration, rather than using it to produce a reduced value. Obviously this is not a major issue: but a mapSeries would make my code more semantially accurate and readable, and would've saved me 15 minutes of reading to figure out how to accomplish what I needed.

@ericclemmons

This comment has been minimized.

ericclemmons commented Nov 13, 2014

Yes, the use-case is legitimate, so I'm not sure why the author is against it's implementation.

We have a lot of Promise.reduce (to perform series) and Promise.reduce alls to aggregate, so our code's actually less clear.

Luckily, we've been able to drop the library entirely by switching to --harmony and yield'ing the results.

Hopefully this semantic improvement lands for the rest of you guys, though!

@bendrucker

This comment has been minimized.

bendrucker commented Nov 14, 2014

@yourcelf For your case you have the concurrency option on map. There's also each which runs in order that can be used for side effects (the return values are not used). Sounds like each is what you need since you're running tests (i.e. the result of the callback should either be an exception or nothing).

This isn't a case of a legitimate need being refused for no reason. If you read through the pages of comments asking for a series or sequence method you'll see that most of the use cases are imagined and once you try to write real code it's not helpful.

@spion

This comment has been minimized.

Collaborator

spion commented Nov 14, 2014

I've actually changed my mind regarding mapSeries. In practical everyday use in my code, there have been more cases where the mapping operation is done in series to avoid race conditions, compared to cases where there is a big benefit from parallelism. Unfortunately the concurrency option of .map is kinda ugly to read because its far from the start

things.map(function(item) {
  // do things with item
  // and then
  return result;
}, {concurrency: 1})

So yeah, I've often found myself wishing there was a mapSeries method.

@petkaantonov

This comment has been minimized.

Owner

petkaantonov commented Nov 14, 2014

This is what each used to do

function mapSeries(things, fn) {
    var results = [];
    return Promise.each(things, function(value, index, length) {
        var ret = fn(value, index, length);
        results.push(ret);
        return ret;
    }).thenReturn(results).all();
}

yourcelf added a commit to yourcelf/bluebird that referenced this issue Nov 14, 2014

Document serial nature of .each more explicitly
In issue petkaantonov#134, I learned more about different ways to achieve something like ``mapSeries`` with bluebird.  I had previously read the docs for ``.each``, but it wasn't clear to me from the docs that it operated serially.  Just add a quick note to make that clear for any others like me who were searching for some serial method.
@benjamingr

This comment has been minimized.

Collaborator

benjamingr commented Feb 9, 2015

@MaffooBristol probably you're missing Promise.each although your example is missing the point of promises - you should always promisify at the lowest level possible.

@MaffooBristol

This comment has been minimized.

MaffooBristol commented Feb 9, 2015

I deleted my comment when I glanced about 2cm up and saw that that is what .each() does, but kudos for a lightning fast reply!

And I do have the majority of my promises at a low level but there's an instance where I want to use it just to control initialisation events (in Marionette) that include both promises and non-promises, so to be able to combine it all in one whilst maybe sacrificing some not-exactly-what-they're-made-for promises seemed a good idea!

Edit: Actually, I read the .each() documentation and it doesn't act in any way like async's .eachSeries(), which I think is kinda a shame because even though bluebird is of course not just a standard async library, it would be nice to be able to define arbitrary callback functions as well as those that make full use of promises with that kind of simple, linear syntax. But maybe there are reasons not to...

@adamreisnz

This comment has been minimized.

adamreisnz commented Jul 8, 2016

The only gripe I have with reduce is the added verbosity due to the introduction of the accumulator and initial value parameters, e.g.:

Promise.reduce(operations, (i, [query, update]) => {
  return Model.update(query, update);
}, 0);

Versus what a cleaner series method could be:

Promise.series(operations, ([query, update]) => {
  return Model.update(query, update);
});

This avoids having to accept the accumulator as a first parameter in the reducer function and makes the api behave the same like for example the map function, where the item from the array is passed as the first parameter.

If all you want to do is execute two promise operations in series, e.g. after one another, then you don't need the accumulator and initial value parameters and they are just in the way with the reduce api.

@benjamingr

This comment has been minimized.

Collaborator

benjamingr commented Jul 8, 2016

This is what Promise.each does

@benjamingr benjamingr reopened this Jul 8, 2016

@benjamingr benjamingr closed this Jul 8, 2016

@adamreisnz

This comment has been minimized.

adamreisnz commented Jul 9, 2016

Ah thanks 👍

@CodeMedic42

This comment has been minimized.

CodeMedic42 commented Dec 18, 2016

Here is something I needed. I did not see anything that did exactly what I wanted so I created this. If there is something like this already please let me know, Sometimes it is hard to find what you are looking for and I really hate recreating the wheel. If not then let me know what you think. I always appreciate feedback.

const Promise = require('bluebird');
const _ = require('lodash');

function series(iterable, inital) {
    return _.reduce(iterable, (accumulator, iteree) => {
        return accumulator.then((result) => {
            return iteree instanceof Function ? iteree(result) : iteree;
        });
    }, Promise.resolve(inital));
};

function X(result) {
    return result + 'X';
}

function Y(result) {
    return result + 'Y';
}

function Z(result) {
    return result + 'Z';
}

const seriesArray = [X, Y, Promise.method(Z)];

series(seriesArray, 'W').then((result) => {
    console.log(result); // => 'XYZ'
});

Lot's of room for improvement but can seriously clean code up and make it more readable I think.

What this does is take an array (can be improved to just take params). With this array it creates a promise chain where the output from the first goes into the next and then the output from that goes into the next. If there is a catch anywhere in the chain it should be bubbled out. It can be initialized with a starting value. The array can have promises, functions, or even static values/objects. They all just feed into the next item in the array. The first item in the array is the final output of the series. Obviously this can be used if you want your items to run in a particular order.

@benjamingr

This comment has been minimized.

Collaborator

benjamingr commented Dec 18, 2016

Here is something I needed. I did not see anything that did exactly what I wanted so I created this. If there is something like this already please let me know,

Read the above comments, there is.

@CodeMedic42

This comment has been minimized.

CodeMedic42 commented Dec 18, 2016

@benjamingr

I'm sorry sometimes I miss things. For example the particular comment you are referring to. I just re-read the above and did not find the comment in particular which you are referring to which solved the particular problem I was attempting to solve with my example.

I looked into the documentation for "each", "map", and "mapSeries" and they all return an array. If I am incorrect about this please let me know which one of these do not return an array so I can re-review the documentation. Either way I was not interested in an array but the result of the last item being ran in the series. This is also the result which I feel is correct when the word "series" is used.

However, the "reduce" function would do the trick but it is, and appropriately as well, designed to be generic when it comes to the use of the accumulator. But this requires a mutli-line blob of code to implement a function which can run particular artifacts in series one after the other passing the result through.

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