New .each behaviour and implicit returns

[joepie91]

In 3.0, the behaviour of each was changed to behave like a sequential map. I feel this was a mistake. The following code:

Promise.each(["a", "b", "c"], item => console.log(item) ).then(result => console.log(final) )

... will result in final holding [undefined, undefined, undefined], which is not what one would expect for an iterator that is meant to be used for side-effects.

> Promise.each(["a", "b", "c"], item => console.log(item) ).then(result => console.log(result) )
Promise {
  _bitField: 0,
  _fulfillmentHandler0: undefined,
  _rejectionHandler0: undefined,
  _promise0: undefined,
  _receiver0: undefined }
> a
b
c
[ undefined, undefined, undefined ]

This problem occurs not only with ES6 arrow functions, but also with implicit returns in other languages like CoffeeScript. I'd consider this a major footgun, as virtually no side-effect-y methods will return the original value.

Should this 'sequential map' behaviour not simply have moved to a new, separate method, without affecting the behaviour of each?

[benjamingr]

It was actually a very common request for each to return the result.

The reason it is "surprising" for each to be chainable is that a lot of languages and constructs have lazy semantics, do not evaluate the results until there is an enumeration and then a method like each causes an enumeration executing the chain - this is not the case with promises which are eager.

If you don't need the return value you can just not use it - did you have code that explicitly relied on each returning undefined?

[joepie91]

Previously, .each returned the previous value without modifying it (like .tap would), ie. ignoring the return value of the callback. This is rather important, because the point of a side-effect is that it shouldn't modify the data you're working with.

Behaviour in Bluebird 2.x:

> Promise.each(["a", "b", "c"], item => console.log(item) ).then(result => console.log(result) )
a
b
c
Promise {
  _bitField: 0,
  _fulfillmentHandler0: undefined,
  _rejectionHandler0: undefined,
  _progressHandler0: undefined,
  _promise0: undefined,
  _receiver0: undefined,
  _settledValue: undefined,
  _boundTo: undefined }
> [ 'a', 'b', 'c' ]

When using implicit returns, it's very easy to return undefined or some other kind of value, and accidentally do a transform while that is not intended (as, after all, it was meant for side-effects).

I can't see why one would request for each to return the result - that's what map is for. The actual intention to me would seem to be a sequential map, which was indeed requested elsewhere, and which should have been provided as a separate method (or option?), rather than replacing each.

[petkaantonov]

This is intended and was one of the most requested features

If you want to reuse the array, use .return

[joepie91]

This does not cover the side-effect case.

Promise.try(=> {
    return ["a", "b", "c"];
})
.each(item => console.log(item) )
.then(final => console.log(final) )

The ["a", "b", "c"] is a simplified value here. Assume that it looks like this instead:

Promise.try(=> {
    return somethingThatResolvesToAnArray();
})
.each(item => console.log(item) )
.then(final => console.log(final) )

There is no way .return will prevent final from becoming [undefined, undefined, undefined] here.

sven@linux-etoq:~/npm-test> node bluebird-each.js 
a
b
c
[ undefined, undefined, undefined ]

Again: why would this behaviour be intended? What is there to gain from a .each that honours return values, beyond a serial map which could have been implemented as a separate method?

[petkaantonov]

Because serial map is very useful and can be used for side effects just by returning the original values as well, without having to implement 2 separate methods

[joepie91]

But this isn't a serial map. It's each. And this is not desirable default behaviour, as it will cause unexpected results with anything that has implicit returns (which is a growing set of tools and preprocessors).

You could even add a serial option to the existing map method (doesn't it already do this with {concurrency: 1} anyway?). I don't see a reason to make each behave like map - that's what map is for.

[petkaantonov]

{concurrency: 1} isn't serial, it takes whatever item finishes first regardless of its place in the input array

[joepie91]

Right. So why not add a {serial: true} option to .map (as there's already an options parameter anyway), and leave .each compatible with implicit returns from side-effects?

[petkaantonov]

Then you would have {concurrency: 5, serial: true} or just {serial: true} (with implicit concurrency: Infinity) and those don't make any sense as it cannot be both concurrent and serial at the same time

[joepie91]

Having mutually exclusive options is not unheard of - in fact, it's made possible by each option being optional.

An extra line of "you can't use these together" documentation (if that - if it doesn't make sense, people likely won't even try it) is certainly a better solution than unexpected behaviour and loss of functionality in every-day usage, in my opinion.

[petkaantonov]

I don't see how there is loss of functionality, what is essentially a useless return value (the same array you were passed) is replaced with a possibly useful one that you can discard if you have no use for it.

[joepie91]

The loss of functionality is the ability to cause side-effects with a .tap-like mechanism, but as an iterator. The "fall-through" behaviour that .each had in 2.x was extremely useful for that purpose.

I also just realized that the problem isn't even just with implicit returns - even if you return nothing, that will of course be treated as undefined, and destroy your input, preventing fall-through.

The only way to mitigate this, is to explicitly return the original value from the .each...

Promise.try(function() {
    return ["a", "b", "c"];
}).each(function(item) {
    console.log(item);
    return item;
}).then(function(final) {
    console.log(final);
})

... which 1) defeats the point of it being an each rather than a map, and 2) is extremely easy to forget (and it fails unsafe). Hence it being a footgun.

[nikkiii]

The way I see it is that it breaks current, existing code and also the learning curve. Users migrating to bluebird might not realize that each does not simply iterate the results, but in fact MAPS them, causing undesired functionality. I'm not sure if you're able to assign new methods to the Promise object, but maybe you're better off with a mapSeries function rather than modifying an existing feature that could cause more headaches than you need.

[petkaantonov]

The loss of functionality is the ability to cause side-effects

This functionality is still there, when you use .each for side effects you don't care about the return value. Even forEach returns undefined and it doesn't stop you from using it for side effects.

[VictorioBerra]

+1 for not changing the existing functionality of .each

[joepie91]

This functionality is still there, when you use .each for side effects you don't care about the return value. Even forEach returns undefined and it doesn't stop you from using it for side effects.

But this change doesn't make it return undefined. It returns transformed values, which could be undefined, or could not be. You don't know, and if you guess wrong, you have an open failure.

The functionality as I described is not 'still there'. You can no longer let the original array fall through. You need to manually work around it.

[petkaantonov]

my point is that it doesn't matter what it returns if you are not going to use the return value

if you find the return value of the same array you already have useful, then that's a minor inconvenience, not some loss of functionality

[joepie91]

if you find the return value of the same array you already have useful, then that's a minor inconvenience, not some loss of functionality

This is 1) the logical behaviour (especially given the heavily chained model of Bluebird), 2) the previous behaviour, and 3) the consistent behaviour (ie. equivalent of .tap for iteration).

On the other hand, we now have a mapSeries pretending to be an each, a loss of the fall-through each functionality, and a massive footgun in every-day usage (in particular for existing users who have come to expect certain behaviour).

I see no reason not to change this.

EDIT: It's also certainly not a 'minor inconvenience'. Bluebird is typically intertwined with your entire codebase, and side-effects in iteration are an extremely common scenario.

[joshmanders]

Not sure about anyone else, but if I want to map, I use .map() not .each(). 👍 agree with @joepie91

[petkaantonov]

It doesn't matter what the return value is if you are not going to use it, if you use .each for side effects then you can do it regardless of what it returns. Returning the same thing you already have is almost by definition not functionality but pure convenience.

[joepie91]

It doesn't matter what the return value is if you are not going to use it, if you use .each for side effects then you can do it regardless of what it returns. Returning the same thing you already have is almost by definition not functionality but pure convenience.

As I've already explained several times, this is the logical and consistent behaviour, and an extremely common requirement for .tap-like scenarios. I don't understand why you deny that this is 'functionality', or why this is even still a discussion to begin with - there's a very clear issue here that is trivial to solve.

The point of .each is that it ignores the return value. That's why you use it instead of .map. Bluebird currently breaks that expectation.

[petkaantonov]

The point of .each is that it ignores the return value.

This wasn't the point of .each at all. In fact, the return value was originally chosen because it felt trivially more useful than returning undefined like forEach does. It was never considered having any functionality, just something that is a tiny bit nicer than undefined. The whole point of .each has always been sequential execution, and now the return value has been made more useful.

That's why you use it instead of .map.

Again, map is only for concurrent execution and managing concurrency.

[joepie91]

This wasn't the point of .each at all. In fact, the return value was originally chosen because it felt trivially more useful than returning undefined like forEach does.

I am not just talking about Bluebird's each implementation, but about the general concept of forEach vs. map in Javascript - even for Array.forEach, the return values are ignored, it just defaults to something else.

It was never considered having any functionality, just something tiny bit nicer than undefined.

So why throw that away?

The whole point of .each has always been sequential execution, and now the return value has been made more useful.

This isn't sequential execution. This is sequential mapping. That is something considerably different, and belongs in its own method. Whether that is map with an option or a new mapSeries method, I don't particularly care.

As it stands, each is either mis-named or mis-implemented, depending on how you look at it.

Again, map is only for concurrent execution and managing concurrency.

It is not. Array.map (and pretty much every other map implementation in JS?) functions sequentially. There is no reason why Bluebird should deviate from that expectation. But it makes sense in asynchronous code to deviate from it by default, and that can be easily solved by having an optional sequential variant.

[spion]

I mostly agree with @joepie91 - I think we need to own up to the fact that we're going the utility grab-bag direction here and just provide mapSeries (non-parallel map, in order), each (tap-like behavior) and map (no order guarantees, concurrency options) separately. Reducing the number of methods in this particular case only adds to the confusion :(

[petkaantonov]

I don't think separate tap-like each is even worth its own method if a mapSeries is available.

[joepie91]

I don't think separate tap-like each is even worth its own method if a mapSeries is available.

It wouldn't have to be - that was already the old behaviour of each, which could simply be restored. We'd then end up with each (tap-like), map (parallel map), mapSeries (sequential map).

[petkaantonov]

In 2.x there was no .mapSeries that's why it's not the same situation.

[ggVGc]

As a bystander having not really heard much about Bluebird before, reading this thread and petkaantonov's behaviour in it has solidified in my mind that the next time I go looking for a javascript library it will not be from here.
Breaking existing code for with the attitude of "but we feel like it and think this new behaviour is just a bit nicer", is very irresponsible, arrogant, and childish. When that same breakage also means breaking with the very accepted definition of a term(in this case "each"/"forEach"), it's even more so, and also to me shows a lacking understanding of the problem domain and basic theory within it, which at least for me makes me lose all confidence that the author has much idea of what he/she is doing in regards to this specific domain.

[spion]

@petkaantonov the name makes no sense, thats the problem. Its worth having another method in this case just for clarity.

[petkaantonov]

@spion ok, let's see what Benjamin says as well

[bergus]

I'd vote for deprecating .each (whose name is way too confusing) and creating a new mapSeries or serialMap or whatever as well. Let's do this fast, before 3.1. I could see each becoming a major support problem.

Then you would have {concurrency: 5, serial: true} or just {serial: true} (with implicit concurrency: Infinity) and those don't make any sense as it cannot be both concurrent and serial at the same time

Actually, it can - and I'd love that option. Make serial:true ensure that the callback is called on the input items in order (as many people expect it anyway, see #399 #599 #708 and more), and let concurrency control how many of the callback-created promises are maximally awaited at the same time.
The goal should be an API that follows the priniciple of least astonishment, even if that is not always optimal (most performant).

[petkaantonov]

@bergus so what would be the return-input-array-while-executing-serially collection method if each is deprecated?

[joepie91]

Let's do this fast, before 3.1. I could see each becoming a major support problem.

Aside from not being in favour of deprecating each (but I've already explained the reasons for that above), if it were deprecated, that'd be a breaking API change and warrant a major version bump.

EDIT: Unless you mean soft deprecation, that is.

[bergus]

@petkaantonov

what would be the return-input-array-while-executing-serially collection method if each is deprecated?

None. Weren't you arguing yourself that it was not needed? I'm all in for "Thou shalt not use side effects!" btw :-)

Of course, we still can fill it in by

Promise.prototype.each = function(cb) {
     return this.mapSerial(cb).return(this);
}

[petkaantonov]

@bergus I couldn't agree more but this whole thread seems to be opposed to that. Also, deprecation is not even a breaking change while changing .each back to 2.x would be, so that's all good reasons for deprecating it.

[joepie91]

Also, deprecation is not even a breaking change while changing .each back to 2.x would be, so that's all good reasons for deprecating it.

Semver makes it so that a breaking change (and resulting major version bump) is not a problem, though. Especially with one being released so quickly, the impact would be near non-existent.

[benjamingr]

I'm for deprecating (not removing) each if we have mapSeries. I think the whole "each and forEach cause side effects" thing is cargo culting on top of the fact languages like C# have lazy enumerables and forEcah is what actually performs the action. This is not the case in JS with JS arrays or with bluebird.

In JS, you can .map in order to forEach, in Java or C# - your code would never be executed because of laziness.

That said I'm not the only user of this library - so if it is "popular enough" I think the current solution is pretty fine.

[tjconcept]

👍 to @joepie91 for arguing. This was one of the only changes in 3.0 I didn't get at all.

I would actually find it more reasonable if each was non-serial as well (to counter map more precisely) and then for both methods to have an optional serial option (I've never seen a valid use case though).

[pnstickne]

"Overloading" each: bad. Creating a new function, mapSeries, orderedMap (eg): good.

However, each should still remain for the explicit purpose of side-effects which is not a "map". Among other things this makes code intent easier to review. It also makes it easier to correct programmers who incorrectly mix side-effect and non-side effects: this should not be encouraged in normal design.