kriskowal / q

A promise library for JavaScript
MIT License
14.94k stars 1.21k forks source link

Q.all surprisingly manipulates array passed to it #746

Open cefn opened 8 years ago

cefn commented 8 years ago

While it makes sense for the array passed to a then call to be populated with results, as far as I can see, the logic of Q.all overwrites the Promises in the array passed to it without warning, if they happen to already resolve to a value.

This is extremely surprising and has created a fairly serious issue in my own code as the behaviour is undocumented anywhere that I can see.

Because I was not expecting the array of promises I was passing to be manipulated by Q, I was also using it elsewhere, relying on there being a 'then' property, which was failing with a transient error in the case that the code was evaluated after one of the Promises had resolved and Q had chosen to overwrite it.

I believe it is an unnecessary behaviour as Q could easily accept or return an array explicitly to be manipulated in this way, and already passes its own array to the then clause, so seems like over-optimisation to try and eliminate a single array creationh by commandeering the one passed in its function call.

Finally, it is not a feature which is likely to be relied upon anyway, assuming that it is not documented.

The offending line looks like...

promises[index] = snapshot.value;

...in Q.all().

kriskowal commented 8 years ago

Yes, this was a regrettable decision, trying to avoid a usually-unnecessary array allocation. Unfortunately, to fix this requires a backward-incompatible release. As such, the behavior has been altered in the v2 branch. Of course, this is far from the only backward-compatibility break in the experimental version.

https://github.com/kriskowal/q/blob/v2/q.js#L304

cefn commented 8 years ago

Thanks for confirming. Great that a fix is in the mix for next release. Of course a workaround is to clone the array before passing to Q.all so now I've identified the issue it's easy to avoid. However, having this behaviour documented against version 1 would be of value I think.