clue · GitHub

This changeset suggests removing the done() method (not to be confused with then() which remains unchanged!). This is done (hah!) in an attempt to reduce the API surface and make the API less confusing and easier to understand. On top of this, this means our APIs are closer to ES6 promises commonly used in JavaScript (see also #219 and #220).

In particular, I consider this method and its relation to the then() method to be somewhat confusing and this has definitely attracted some bad code in the past. Most notably, this method would be used when you would want to ensure a promise is successfully handled in order to make sure we're not hiding any unhandled rejections.

I strongly believe the solution discussed in #87 is superior as it would ensure all unhandled rejections would be reported in the future Promise v3 by default. On top of this, we're transitioning towards using Fibers and coroutines with https://github.com/reactphp/async, so promises would be consumed using await() or yield for many use cases, which would throw any rejections, thus making it less likely to accidentally hide any exceptions.

Given reporting unhandled rejections as discussed in #87 is currently on the foreseeable roadmap, I see little value in having an additional done() method that has the sole purpose of forcefully halting the program in this case. Using an unclean shutdown is not particularly useful in long-running applications and is probably not what we would want to propose as a solution to handle this situation. If a forceful shutdown is desired (making up use cases at this point, but happy to hear about more relevant use cases!), this can still be implemented in userland:

// old
$browser->get($url)->done(function () {
    echo 'Successfully downloaded!';
});
// still supported
$browser->get($url)->then(function () {
    echo 'Successfully downloaded!';
}, function (Exception $e) {
    die 'Error: ' . $e->getMessage() . PHP_EOL;
});

Empirical evidence suggests this interface isn't used much in our ecosystem either, so I'd rather use the chance to clean up our API surface with the upcoming Promise v3 release.

Like #219 and #220, I'm filing this as an RFC to get more feedback on this method and to see how others feel about this. If this gets merged, I'll file a follow-up PR to deprecate this method for Promise v2 to ease upgrading.

Read the original on github.com ↗