Promise.all indexing issue when combining non-zone.js promises

[taras-mytofir]

When you use Promise.all for combining not only zone.js promises (e.g. WinJSPromise) you can fall into issue that Promise.all results into single element result.

Example of issue https://plnkr.co/edit/UO2QhIa5eeLm6tPAyAjt?p=preview.
Important part of the code(skipping WinJSPromise import) is this one:

  let p1 = WinJSPromise.as(3);
  let p2 = Promise.resolve(45);
  Promise.all([p1, p2]).then(res => {
    console.log(`Promise.all: `, res);
  })
  WinJSPromise.join([p1, p2]).then(res => {
    console.log(`WinJSPromise.join: `, res);
  });

The output from Promise.all is [45], where expected is [3, 45]. This works fine with native Promise.all.

Problem is in how index is counted in this implementation:

zone.js/lib/common/promise.ts

Lines 305 to 331 in c8c5990

	static all<R>(values: any): Promise<R> {
	let resolve: (v: any) => void;
	let reject: (v: any) => void;
	let promise = new this<R>((res, rej) => {
	resolve = res;
	reject = rej;
	});
	let count = 0;
	const resolvedValues: any[] = [];
	for (let value of values) {
	if (!isThenable(value)) {
	value = this.resolve(value);
	}
	value.then(
	((index) => (value: any) => {
	resolvedValues[index] = value;
	count--;
	if (!count) {
	resolve(resolvedValues);
	}
	})(count),
	reject);
	count++;
	}
	if (!count) resolve(resolvedValues);
	return promise;
	}

Following update implementation fixes the issue (note the index variable missing in original version):

Promise.all = function (values: any): Promise<any> {
    let resolve: (v: any) => void;
    let reject: (v: any) => void;
    let promise = new this((res, rej) => {
      resolve = res;
      reject = rej;
    });
    let count = 0;
    let index = 0;
    const resolvedValues: any[] = [];
    for (let value of values) {
      if (!(value && value.then)) {
        value = this.resolve(value);
      }
      value.then(
          ((index) => (value: any) => {
            resolvedValues[index] = value;
            count--;
            if (!count) {
                resolve(resolvedValues);
            }
          })(index),
          reject);
      count++;
      index++;
    }
    if (!count) resolve(resolvedValues);
    return promise;
}

This issue was found and discussed a bit here

[JiaLiPassion]

@taras-mytofir , thank you for posting the issue, I will check it.

[JiaLiPassion]

@taras-mytofir , could you provide a reproduce repo?
I just added a test case like this.

            const otherPromise: any = {
              then: function(thenCallback: (value: string) => any) {
                thenCallback('otherPromise');
              }
            };
            Promise.all([Promise.resolve('resolution'), otherPromise]).then((v: any) => value = v);
            expect(value).toEqual(['resolution', 'otherPromise']);

And the test passes.

[taras-mytofir]

@JiaLiPassion example with one promise as WinJSPromise:
https://plnkr.co/edit/biopXhYkyxK9hhmQxlVh?p=preview

[JiaLiPassion]

@taras-mytofir, the code you provided failed because WinJSPromise not following the Promise Spec which was described here https://promisesaplus.com/#point-67, promise.then should make sure run in async way, so the callback which was passed into then should not run in the current tick turn.

So I think you should report bug to WinJSPromise. Or you can patch WinJSPromise just like this https://plnkr.co/edit/yUVkEdTecvc0jKO8fqdG?p=preview (BTW, this is just a quick patch, just to describe the idea).

[taras-mytofir]

I see, thanks for looking into this!

[lightrabbit]

This problem also occurs on the API of the Universal Windows Platform. Although WinJSPromise and UWP API do not follow Promise spec. But in order to avoid confusion for developers, making the performance of ZoneAwarePromise the same as native Promise, is it a better choice?

For example, the test case(non standard Promise) below:

class MockPromise {
    constructor(value) {
        this.value = value;
    }
    then(resolve, reject) {
        resolve(this.value);
    }
}
var a = new MockPromise('a');
var b = new MockPromise('b');
Promise.all([a,b]).then(values=>console.log(values));

For native Promise, the result is

["a", "b"]

For ZoneAwarePromise, the result is

["b"]