Reject a join on no parameters.

[benjamingr]

Fixes #1218

[overlookmotel]

How about the case where only a single promise and no callback is passed to Promise.join()?

Promise.join( Promise.resolve( 1 ) ).then( function() { ... } );

I think the if (arguments.length === 1) line will create an error in this case, even though it's legal.

Maybe move the test for no promises to after if (last > 0 && typeof arguments[last] === "function") {?

[benjamingr]

@overlookmotel I'm modifying it to reject whenever a function is not passed as a last parameter - or if only a function is passed and no promises/values.

[benjamingr]

Please take a look at the changes.

[spion]

I don't think we should do this last bit - the amount of breakage is not worth it. Nothing wrong on join working like all when a function has not been passed.

[petkaantonov]

Agree with spion that we should keep the old behavior and only throw if nothing but single immediate function value is passed

[benjamingr]

The current behavior is confusing and causes a lot of issues for people. I absolutely think breakage is fine when we have a method that has been deprecated for many years (since before 2.0).

[spion]

There is no good reason to deprecate either. Promise.join works just fine the way it is, baring few minor bugs.

Lets not make changes just for the sake of making them.

[benjamingr]

@spion it's very confusing to have a function that does two separate things for the convenience of not having to write [].

[spion]

It does not do two separate things. The default function, if not provided is just () => [].slice.call(arguments)

edit: this is from an API user's perspective

[benjamingr]

It's deprecated behavior for several years, people get confused with it all the time, it has edge cases where it doesn't work (see the issues here) and I see no reason why we should support it given it's just as easy to use .all which is standard.

Note that .join only has a performance benefit when you pass the function anyway, so this is just a function that exists in the API to avoid writing a single pair of []s while being non-standard, and having a confusing case where a single function is passed as an argument.

Personally I would probably remove it altogether as well as anything that's not useful anymore with async/await and coroutines everywhere.

[petkaantonov]

There is no reason to break existing code because the docs only mention the old usage at the end as side note so the risk of new code using the old syntax is small, not worth the breakage.

[benjamingr]

@petkaantonov I see code that breaks in the wild no matter what modification we make to join, there is code in the wild relying on the fact that a single function argument pends forever, and code in the wild that relies on the fact Promise.join() is a resolved promise for an array.

I think the point of a semver major is to allow for these cleanups.

We will always break someone's workflow

(I did a GH code search for Promise.join, it's in the first few pages).

[petkaantonov]

If there's really code relying on function never being called we should not make that change either

[benjamingr]

There is. To be clear - I'm not suggesting we merge this into 2.x, I'm suggesting we aim for 4.x, we made much more breaking changes in 2.x -> 3.x breaking a lot more code and I think it was for the better.

Stuff like "swapping the argument order of delay" is a lot more breakage than "in edge cases and behavior that's been deprecated for join bluebird will not fail fast and not do quirky things".

It's your philosophy for the library of being genuinely useful and not "more correct or compatible" that I'm arguing for here - in Node for example it would stay broken for life.

[spion]

I still haven't seen a valid reason why this is confusing. The function takes an optional function argument, which, if not present, defaults to a function that places all arguments into an array.

Can someone explain how is this confusing? Whats the exact mechanism here that causes confusion?

And please don't make comparisons with an accidental edge case either. This behaviour has been specifically intended in the library for a long, long time, and infact, it was the only intended behaviour at the beginning.

So to clarify: I'm all for fixing unintended behaviour, even if it causes breakage. But I'm against deprecating and removing intended behaviour unless there is a very good reason for it. And the explanation has to be better than "its confusing", because its possible to say that about anything.

[benjamingr]

Promise.join(function() { //should I resolve with a promise for the function? Reject? The two cases conflict
});

Promise.join(a, sometimesFunctionSometimesPromise); // like Array(3) vs [3], also `then`s would fire differently

Also, it does the same thing .all does for the first case. I think it was a mistake to support both cases at once.

[spion]

1. Must pass at least one promise
2. Whats wrong with it? Can you explain how is it like Array(3) vs [3]. And how about a realistic example of a variable that sometimes contains a function and other times a promise?