stefanpenner/es6-promise

`Promise.all` should not be concerned with `this`

mlanza opened this issue · 1 comments

mlanza commented

It is typical to be awaiting the completion of several promises somewhere upstream in your promise chain. Assuming your prior step has an array of promises the natural thinking would be to simply then into a Promise.all; however, that does not work.

Promise.resolve([p1, p2, p3])
    .then(Promise.all)

It can be made to work by binding the Promise to the all method, but this is not immediately obvious when you're thinking the above will work. I would guess most developers would naturally try the above before running into an issue and then having to adjust.

Promise.resolve([p1, p2, p3])
    .then(Promise.all.bind(Promise))

This works too, but this is even worse.

Promise.resolve([p1, p2, p3])
  .then(function (promises) {
     return Promise.all(promises);
   })

The root of the issue is the this in the all. By including a this, all becomes a method rather than a function. It is impure because this is a non-explicit parameter.

function all(entries) {
  return new Enumerator(this, entries).promise;
}

Is there a realistic use case for binding to a different this context? If not, I would recommend replacing the this with a reference to Promise.

stefanpenner commented

Is there a realistic use case for binding to a different this context? If not, I would recommend replacing the this with a reference to Promise.

The behavior you see is part of the language specification -> [25.4.4.1.1 Promise.all] (https://tc39.github.io/ecma262/#sec-promise.all)

Specifically the first two lines:

  1. Let C be the this value
  2. If Type(C) is not Object, throw a TypeError exception

The reason for this, is to support inheritance correctly. For example:

class MyNewAmazingPromise extends Promise {};

MyNewAmazingPromise.all([1])) instanceof MyNewAmazingPromise // => true, with the spec
MyNewAmazingPromise.all([1])) instanceof MyNewAmazingPromise // => false, with your recommendation

You may though notice Array.isArray does not suffer from this, but if I remember correctly the language authors felt that was a misstep.

If you wish to destructure all it can be but must be first bound.

const all = Promise.all.bind(Promise);

I am closing this issue as invalid. If I misunderstood something, please let me know. Also feel free to continue the discussion.

  • 👍1