kriskowal / q

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

Validate input of Q.all #700

Open foretspaisibles opened 9 years ago

foretspaisibles commented 9 years ago

When it called with something else than an array of promises, the static method Q.all should fail with a TypeError error condition.

A common pitfall when using the library is to call Q.all on an array filled with return values of Q.defer() instead of the promise member of these values.


It might also be argued that Q.all should return a failed promise, but it seems to me that an exception is more appropriate for what seems like a program logic error.

vingiarrusso commented 9 years ago

@michipili Check out the conversation on my pull request #667. You have a handful of failing tests with this change (#701), due to strictly checking for an array as the parameter. Its kind of a weird situation where you know you want an array but even the library itself passes .all() an array-like (see promised decorator fn).

foretspaisibles commented 9 years ago

@vingiarrusso I see, I overlooked that CONTRIBUTE.md file. The main contribution in my branch is the use of isPromise to validate items of the array-like arguments, maybe you would like to build this in your own branch? The call would then be

Array.prototype.every.call(arrraylike, isPromise)

instead of

truearray.every(isPromise)

In the code, there is a validation which does not happen on each execution paths, this should be fixed.

Refs: https://github.com/michipili/q/commit/9518c0588469522007d26f5d2ae62fb080c1b536#diff-a148bc52a39c990d02038097d8177455R1564

cefn commented 9 years ago

Can I request also that there is a useful failure condition when calling Q.all with 'promises' with the value undefined.

Q.all([void(0)])

I recently suffered for several days from this bug as Q.all was silently resolving when I did this...

                    return Q.all(keys.map(function(key){
                            tree.syncRecursive(getChildTopic(topic, key));
                        }))
                        .then(function(){
                            return tree.getItem(topic);
                        })

...instead of this...

                    return Q.all(keys.map(function(key){
                            return tree.syncRecursive(getChildTopic(topic, key));
                        }))
                        .then(function(){
                            return tree.getItem(topic);
                        })

Spot the difference ;)