reactphp / promise

Promises/A implementation for PHP.
https://reactphp.org/promise/
MIT License
2.38k stars 146 forks source link

[RFC] Consider deprecating FulfilledPromise and RejectedPromise (mark as internal only) #155

Closed clue closed 4 years ago

clue commented 4 years ago

What purpose does the FulfilledPromise and RejectedPromise have? It's my understanding these should be used internally only and using the resolve() and reject() functions is the preferred alternative (or in many cases simply omit it).

For instance, here's some example code that highlights how using these classes is often unneeded:

fetch($url)->then(function ($response) {
    if ($response->getStatusCode() !== 200) {
        return new RejectedPromise(new RuntimeException());
    }
    return new FulfilledPromise($response);
});

The following code solves the exact same use case without any explicit references to these classes:

fetch($url)->then(function ($response) {
    if ($response->getStatusCode() !== 200) {
        throw new RuntimeException();
    }
    return $response;
});

Likewise, the following lines also have the same effect:

$promise = new FulfilledPromise($value);
$promise = resolve($value);
$promise = new RejectedPromise($reason);
$promise = reject($reason);

Do we have valid use cases where it makes sense to expose these classes? I'd love to see some input on this.

If we can't come up with valid use cases, I would suggest deprecating both classes for v2.x and removing them in v3.0 from our public API (which might mean marking them as @internal only). This way, we can reduce our API surface and are more in line with other promise implementations (e.g. ES6 promises). This simplifies our documentation and helps steering people to recommended usage patterns.

What do you think?

WyriHaximus commented 4 years ago

@clue I was kinda expecting this to be a PR I could instantly approve ❤️

jsor commented 4 years ago

Those 2 classes have been implicitely made part of the public API with the 2.0 release by adding it to the documenttation (see #12).

It makes perfect sense to revert that, so 👍 from me.

mmoreram commented 4 years ago

Would you introduce a deprecated message? I'm not really sure how to do it. Maybe looking for the caller?

clue commented 4 years ago

@mmoreram I have no plans to introduce any "deprecation message" (where should this be reported to?). Instead, I've marked both classes as deprecated via #165 for Promise v2 and both will be marked as internal via #164 for Promise v3.

WyriHaximus commented 4 years ago

Closing this as both PR's have been merged.

mmoreram commented 4 years ago

@clue deprecation logs with flag E_DEPRECATED