Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
clue opened this issue Dec 8, 2019 · 6 comments
Labels
Milestone

Comments

@clue
Copy link
Member

clue commented Dec 8, 2019

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
Copy link
Member

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

@jsor
Copy link
Member

jsor commented Dec 9, 2019

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
Copy link

mmoreram commented Mar 8, 2020

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

@clue
Copy link
Member Author

clue commented Mar 8, 2020

@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
Copy link
Member

Closing this as both PR's have been merged.

@mmoreram
Copy link

mmoreram commented Mar 8, 2020

@clue deprecation logs with flag E_DEPRECATED

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants