Require argument for resolve() function

[clue]

This changeset updates the resolve() and $deferred->resolve() signature to require a value instead of having a null default value. This is mostly done for consistently with how ES6 promises in JavaScript also always require this argument and how our reject() signature now always requires a Throwable as of #138.

// unchanged
$promise = resolve($value);
$deferred->resolve($value);

// old
$promise = resolve();
$deferred->resolve();

// new
$promise = resolve(null);
$deferred->resolve(null);

Most promises would usually be resolved with some kind of value, so this shouldn't affect most consumers. By requiring this as an input argument we can be more type safe with how the promise API always fulfills with a value as an output (refs #188).

[jsor]

Makes perfect sense. 👍

FTR: As the argument passed to resolve is equivalent to the return value of a function, the intention was, that calling resolve() without an argument is the equivalent of :void (in JavaScript you can resolve with undefined instead of null to signal that).

By requiring this as an input argument we can be more type safe with how the promise API always fulfills with a value as an output (refs #188).

Also, keep in mind that you can resolve a Promise with another Promise, in which case it resolves to the same value as the followee.

[Seldaek]

Just noticed this change while updating composer/composer#10429 to the latest 3.x-dev state here.

I am not a fan because it leads to tons of breakage and I worry this will especially break third party code (composer plugins) when we upgrade the bundled react/promise to v3. As we have a bunch of optionally-returned promises due to BC concerns, it's common in some places to do $promise = something(); if (!$promise) { $promise = resolve(); } $promise->then(...)

Maybe it'll all be fine and I am overreacting, I don't know.. Just wanted to voice the concern :)

The forward-compat way when using 2.x would be to already call resolve(null) is that correct?

[jsor]

The forward-compat way when using 2.x would be to already call resolve(null) is that correct?

Correct.

[clue]

I am not a fan because it leads to tons of breakage and I worry this will especially break third party code (composer plugins) when we upgrade the bundled react/promise to v3. As we have a bunch of optionally-returned promises due to BC concerns, it's common in some places to do $promise = something(); if (!$promise) { $promise = resolve(); } $promise->then(...)

@Seldaek Thanks for bringing this up and raising your concerns! It's correct that the above snippet would not work with Promise v3 as suggested in this PR. If you update resolve() to resolve(null), then everything should work with Promise v1 + v2 + v3.

The idea is to bring this library more in line with how promises work in other ecosystems and also because it makes a lot of sense when considering how promises wrap around synchronous concepts (see #188 for future type safety). For instance, JavaScript also requires an argument: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/resolve

Maybe it'll all be fine and I am overreacting, I don't know.. Just wanted to voice the concern :)

Don't worry, your input is much appreciated. I'm definitely open to discuss this and would love to learn more about the BC impact this has on the larger ecosystem.

[Seldaek]

Don't worry, your input is much appreciated. I'm definitely open to discuss this and would love to learn more about the BC impact this has on the larger ecosystem.

We won't know that until I ship composer with react/promise 3.x and things start breaking (or not), so I guess I'll get back to you then if needed.