\React\Promise\all not preserving array key order

[lordthorzonus]

Hi I ran into an strange issue and am wondering is this intended behavior?

Basically I'm expecting that for the included code the output would be:

.Array
(
    [0] => 1
    [1] => 2
    [2] => 3
)

But instead I'm getting the array keys unordered:

.Array
(
    [0] => 1
    [2] => 3
    [1] => 2
)

Which causes problems for example when json_encoding (PHP thinks it should be encoded as an object not an array).

Code to reproduce:

$loop = Factory::create();

$testData = [
    new Promise(
        function ($resolve) use ($loop) {
            $loop->nextTick(
                function () use ($resolve) {
                    $resolve(1);
                }
            );
        }
    ),
    new Promise(
        function ($resolve) use ($loop) {
            $loop->nextTick(
                function () use ($resolve, $loop) {
                    \React\Promise\resolve()->then( function() use ($resolve, $loop) {
                        $loop->nextTick(function () use ($resolve) {
                            $resolve(2);
                        });
                    });
                }
            );
        }
    ),
    new Promise(
        function ($resolve) use ($loop) {
            $loop->nextTick(
                function () use ($resolve) {
                    $resolve(3);
                }
            );
        }
    )
];


\React\Promise\all($testData)->then(function ($result) {
    print_r($result);
});

$loop->run();

Thanks!

[arosolino]

The order of the values correlates to the order they are resolved. Couldn't you just sort the result?

\React\Promise\all($testData)->then(function ($result) {
    ksort($result);
    print_r($result);
});

Or if the order doesn't matter just proper JSON encoding just pull the values

\React\Promise\all($testData)->then(function ($result) {
    print_r(array_values($result));
});

[vladar]

@arosolino this explaination of ES6 spec is in favor of strictly preserving original ordering of elements.

But even without ES6 spec... It seems useless to order final array after the order of promise resolution (as it is never guaranteed, which basically means that final array is returned sorted randomly). Is there any value in preserving the order of promise resolution vs original ordering?

[lordthorzonus]

@arosolino @vladar

And the original order is also preserved simulating the same kind of test in chrome with JS:

let p1 = new Promise((resolve, reject) => {
    window.setTimeout(() => {
        resolve(1);
    }, 180);
});
let p2 = new Promise((resolve, reject) => {
    window.setTimeout(() => {
        Promise.resolve().then(function () {
                window.setTimeout(function () {
                    resolve(2)
                }, 250)
            }
        )
    }, 250);
});
let p3 = new Promise((resolve, reject) => {
    window.setTimeout(() => {
        resolve(3);
    }, 200);
});

Promise.all([p1, p2, p3]).then(values => {
    console.log(values); // [1, 2, 3]
});

[arosolino]

If this needs to be done, then how about this.

https://github.com/reactphp/promise/blob/master/src/functions.php#L143

Instead of making a new $values array why not just overwrite each $promisesOrValues element with the resolved value. It would preserve order.

[jsor]

Yes, order is not preserved at the moment, only keys are preserved.

The order of the values correlates to the order they are resolved.

Yes, that is correct.

I'm not sure changing this would be a BC break, eg. for someone relying on that ordering. I'd like to hear opinions of @clue and @WyriHaximus on this.

[lordthorzonus]

@jsor I'm happy to provide a PR if this can be changed :). I think nobody can rely on the order being random though currently?

[jsor]

Yes, relying on the ordering would be very weird. But we're very conservative with BC. The question is not about the change itself but whether that should go into 2.x or 3.x.

[arosolino]

To preserve BC you could pass an extra parameter to all(), and then to map() for $preserveKeys.

[clue]

I agree that it makes sense to keep the original key order instead of the order the promises are resolved in 👍 Note that this can't really be compared with JavaScript, because they use array as an ordered list of values so that the key position is indeed significant, whereas PHP uses an hash-map with the original keys so that the key-value-relation is (more) important here.

I'm not sure changing this would be a BC break, eg. for someone relying on that ordering. I'd like to hear opinions of @clue and @WyriHaximus on this.

I'd rather get this merged in the v2 branch, given that this behavior is currently undefined, undocumented and untested, I would not consider this a BC break 👍

[WyriHaximus]

Agreeing with @clue here, this behavior is undocumented and thus changing to keep the array order isn't a BC break. Would also prefer to see this in the v2 branch (and v3 of course), as this is a neat little feature to have and definitely have use cases for this 👍 .

[jsor]

@lordthorzonus Ok, appreciating a PR against the 2.x branch :)

[jsor]

Implemented by @lordthorzonus in #77