Merge pull request #224 from clue-labs/no-done

[RFC] Remove `done()` method

Author: WyriHaximus

README.md

@@ -29,7 +29,6 @@ Table of Contents
      * [Deferred::reject()](#deferredreject)
    * [PromiseInterface](#promiseinterface)
      * [PromiseInterface::then()](#promiseinterfacethen)
-     * [PromiseInterface::done()](#promiseinterfacedone)
      * [PromiseInterface::catch()](#promiseinterfacecatch)
      * [PromiseInterface::finally()](#promiseinterfacefinally)
      * [PromiseInterface::cancel()](#promiseinterfacecancel)
@@ -48,7 +47,6 @@ Table of Contents
      * [Resolution forwarding](#resolution-forwarding)
      * [Rejection forwarding](#rejection-forwarding)
      * [Mixed resolution and rejection forwarding](#mixed-resolution-and-rejection-forwarding)
-   * [done() vs. then()](#done-vs-then)
 5. [Install](#install)
 6. [Credits](#credits)
 7. [License](#license)
@@ -183,28 +181,6 @@ the same call to `then()`:
 
 * [resolve()](#resolve) - Creating a resolved promise
 * [reject()](#reject) - Creating a rejected promise
-* [PromiseInterface::done()](#promiseinterfacedone)
-* [done() vs. then()](#done-vs-then)
-
-#### PromiseInterface::done()
-
-```php
-$promise->done(callable $onFulfilled = null, callable $onRejected = null);
-```
-
-Consumes the promise's ultimate value if the promise fulfills, or handles the
-ultimate error.
-
-It will cause a fatal error (`E_USER_ERROR`) if either `$onFulfilled` or
-`$onRejected` throw or return a rejected promise.
-
-Since the purpose of `done()` is consumption rather than transformation,
-`done()` always returns `null`.
-
-#### See also
-
-* [PromiseInterface::then()](#promiseinterfacethen)
-* [done() vs. then()](#done-vs-then)
 
 #### PromiseInterface::catch()
 
@@ -579,74 +555,6 @@ $deferred->promise()
 $deferred->resolve(1);  // Prints "Mixed 4"
 ```
 
-### done() vs. then()
-
-The golden rule is:
-
-    Either return your promise, or call done() on it.
-
-At a first glance, `then()` and `done()` seem very similar. However, there are
-important distinctions.
-
-The intent of `then()` is to transform a promise's value and to pass or return
-a new promise for the transformed value along to other parts of your code.
-
-The intent of `done()` is to consume a promise's value, transferring
-responsibility for the value to your code.
-
-In addition to transforming a value, `then()` allows you to recover from, or
-propagate intermediate errors. Any errors that are not handled will be caught
-by the promise machinery and used to reject the promise returned by `then()`.
-
-Calling `done()` transfers all responsibility for errors to your code. If an
-error (either a thrown exception or returned rejection) escapes the
-`$onFulfilled` or `$onRejected` callbacks you provide to `done()`, it will cause
-a fatal error.
-
-```php
-function getJsonResult()
-{
-    return queryApi()
-        ->then(
-            // Transform API results to an object
-            function ($jsonResultString) {
-                return json_decode($jsonResultString);
-            },
-            // Transform API errors to an exception
-            function ($jsonErrorString) {
-                $object = json_decode($jsonErrorString);
-                throw new ApiErrorException($object->errorMessage);
-            }
-        );
-}
-
-// Here we provide no rejection handler. If the promise returned has been
-// rejected, the ApiErrorException will be thrown
-getJsonResult()
-    ->done(
-        // Consume transformed object
-        function ($jsonResultObject) {
-            // Do something with $jsonResultObject
-        }
-    );
-
-// Here we provide a rejection handler which will either throw while debugging
-// or log the exception
-getJsonResult()
-    ->done(
-        function ($jsonResultObject) {
-            // Do something with $jsonResultObject
-        },
-        function (ApiErrorException $exception) {
-            if (isDebug()) {
-                throw $exception;
-            } else {
-                logException($exception);
-            }
-        }
-    );
-```
-
 Install
 -------
 

src/Internal/FulfilledPromise.php

@@ -5,7 +5,6 @@
 use React\Promise\Promise;
 use React\Promise\PromiseInterface;
 use function React\Promise\enqueue;
-use function React\Promise\fatalError;
 use function React\Promise\resolve;
 
 /**
@@ -41,25 +40,6 @@ public function then(callable $onFulfilled = null, callable $onRejected = null):
         });
     }
 
-    public function done(callable $onFulfilled = null, callable $onRejected = null): void
-    {
-        if (null === $onFulfilled) {
-            return;
-        }
-
-        enqueue(function () use ($onFulfilled) {
-            try {
-                $result = $onFulfilled($this->value);
-            } catch (\Throwable $exception) {
-                return fatalError($exception);
-            }
-
-            if ($result instanceof PromiseInterface) {
-                $result->done();
-            }
-        });
-    }
-
     public function catch(callable $onRejected): PromiseInterface
     {
         return $this;

src/Internal/RejectedPromise.php

@@ -6,7 +6,6 @@
 use React\Promise\PromiseInterface;
 use function React\Promise\_checkTypehint;
 use function React\Promise\enqueue;
-use function React\Promise\fatalError;
 use function React\Promise\resolve;
 
 /**
@@ -38,29 +37,6 @@ public function then(callable $onFulfilled = null, callable $onRejected = null):
         });
     }
 
-    public function done(callable $onFulfilled = null, callable $onRejected = null): void
-    {
-        enqueue(function () use ($onRejected) {
-            if (null === $onRejected) {
-                return fatalError($this->reason);
-            }
-
-            try {
-                $result = $onRejected($this->reason);
-            } catch (\Throwable $exception) {
-                return fatalError($exception);
-            }
-
-            if ($result instanceof self) {
-                return fatalError($result->reason);
-            }
-
-            if ($result instanceof PromiseInterface) {
-                $result->done();
-            }
-        });
-    }
-
     public function catch(callable $onRejected): PromiseInterface
     {
         if (!_checkTypehint($onRejected, $this->reason)) {

src/Promise.php

@@ -57,19 +57,6 @@ static function () use (&$parent) {
         );
     }
 
-    public function done(callable $onFulfilled = null, callable $onRejected = null): void
-    {
-        if (null !== $this->result) {
-            $this->result->done($onFulfilled, $onRejected);
-            return;
-        }
-
-        $this->handlers[] = static function (PromiseInterface $promise) use ($onFulfilled, $onRejected) {
-            $promise
-                ->done($onFulfilled, $onRejected);
-        };
-    }
-
     public function catch(callable $onRejected): PromiseInterface
     {
         return $this->then(null, static function ($reason) use ($onRejected) {
@@ -151,9 +138,15 @@ private function resolver(callable $onFulfilled = null, callable $onRejected = n
     {
         return function ($resolve, $reject) use ($onFulfilled, $onRejected) {
             $this->handlers[] = static function (PromiseInterface $promise) use ($onFulfilled, $onRejected, $resolve, $reject) {
-                $promise
-                    ->then($onFulfilled, $onRejected)
-                    ->done($resolve, $reject);
+                $promise = $promise->then($onFulfilled, $onRejected);
+
+                if ($promise instanceof self && $promise->result === null) {
+                    $promise->handlers[] = static function (PromiseInterface $promise) use ($resolve, $reject) {
+                        $promise->then($resolve, $reject);
+                    };
+                } else {
+                    $promise->then($resolve, $reject);
+                }
             };
         };
     }

src/PromiseInterface.php

@@ -34,22 +34,6 @@ interface PromiseInterface
      */
     public function then(?callable $onFulfilled = null, ?callable $onRejected = null): PromiseInterface;
 
-    /**
-     * Consumes the promise's ultimate value if the promise fulfills, or handles the
-     * ultimate error.
-     *
-     * It will cause a fatal error (`E_USER_ERROR`) if either `$onFulfilled` or
-     * `$onRejected` throw or return a rejected promise.
-     *
-     * Since the purpose of `done()` is consumption rather than transformation,
-     * `done()` always returns `null`.
-     *
-     * @param callable|null $onFulfilled
-     * @param callable|null $onRejected
-     * @return void
-     */
-    public function done(callable $onFulfilled = null, callable $onRejected = null): void;
-
     /**
      * Registers a rejection handler for promise. It is a shortcut for:
      *

src/functions.php

@@ -88,7 +88,7 @@ function all(array $promisesOrValues): PromiseInterface
             $values[$i] = null;
 
             resolve($promiseOrValue)
-                ->done(
+                ->then(
                     function ($mapped) use ($i, &$values, &$toResolve, $resolve): void {
                         $values[$i] = $mapped;
 
@@ -125,7 +125,7 @@ function race(array $promisesOrValues): PromiseInterface
             $cancellationQueue->enqueue($promiseOrValue);
 
             resolve($promiseOrValue)
-                ->done($resolve, $reject);
+                ->then($resolve, $reject);
         }
     }, $cancellationQueue);
 }
@@ -187,7 +187,7 @@ function any(array $promisesOrValues): PromiseInterface
             $cancellationQueue->enqueue($promiseOrValue);
 
             resolve($promiseOrValue)
-                ->done($fulfiller, $rejecter);
+                ->then($fulfiller, $rejecter);
         }
     }, $cancellationQueue);
 }
@@ -206,19 +206,6 @@ function enqueue(callable $task): void
     $queue->enqueue($task);
 }
 
-/**
- * @internal
- */
-function fatalError($error): void
-{
-    try {
-        \trigger_error($error, E_USER_ERROR);
-    } catch (\Throwable $e) {
-        \set_error_handler(null);
-        \trigger_error($error, E_USER_ERROR);
-    }
-}
-
 /**
  * @internal
  */

tests/ErrorCollector.php

@@ -178,19 +178,6 @@ public function shouldRejectWithoutCreatingGarbageCyclesIfCancellerHoldsReferenc
         $this->assertSame(0, gc_collect_cycles());
     }
 
-    /** @test */
-    public function shouldIgnoreNotifyAfterReject()
-    {
-        $promise = new Promise(function () { }, function ($resolve, $reject, $notify) {
-            $reject(new \Exception('foo'));
-            $notify(42);
-        });
-
-        $promise->then(null, null, $this->expectCallableNever());
-        $promise->cancel();
-    }
-
-
     /** @test */
     public function shouldNotLeaveGarbageCyclesWhenRemovingLastReferenceToPendingPromise()
     {
@@ -212,17 +199,6 @@ public function shouldNotLeaveGarbageCyclesWhenRemovingLastReferenceToPendingPro
         $this->assertSame(0, gc_collect_cycles());
     }
 
-    /** @test */
-    public function shouldNotLeaveGarbageCyclesWhenRemovingLastReferenceToPendingPromiseWithDoneFollowers()
-    {
-        gc_collect_cycles();
-        $promise = new Promise(function () { });
-        $promise->done();
-        unset($promise);
-
-        $this->assertSame(0, gc_collect_cycles());
-    }
-
     /** @test */
     public function shouldNotLeaveGarbageCyclesWhenRemovingLastReferenceToPendingPromiseWithCatchFollowers()
     {