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

Add new sleep() function and deprecate resolve() and reject() functions #51

Merged
merged 2 commits into from
Dec 5, 2021

Conversation

clue
Copy link
Member

@clue clue commented Dec 5, 2021

This changeset adds a new sleep() function and deprecates the resolve() and reject() functions in favor of the new sleep() function.

The new sleep() function does precisely what the current resolve() function does: It creates a promise that will be fulfilled in a given number of seconds. The only difference is that the sleep() function resolves with no value, whereas the resolve() function resolves with the number of seconds. The previous type was a pretty arbitrary design decision I made, the new type now matches how usleep() returns a void.

Using the name sleep() makes a lot more sense as introducing an asynchronous delay is a rather common requirement and it makes sense to mimic PHP's built-in sleep() function. On top of this, this also helps avoid any ambiguities, as the React\Promise\resolve() function does something entirely different than the React\Promise\Timer\resolve() function (same local name, only different namespace). This is also a major DX improvement, as IDE autocompletion would always suggest the wrong function import, let alone the fact that you wouldn't look for a function named "resolve" if you want to "sleep".

Similarly, the reject() function was mostly added to complement the resolve() function, but I don't see any real-world use cases for this. Implementing this on top of the sleep() function is trivial (see its implementation), so I will argue it provides very little value.

Builds on top of #49

@clue clue added this to the v1.8.0 milestone Dec 5, 2021
@clue clue requested a review from WyriHaximus December 5, 2021 15:46
@clue
Copy link
Member Author

clue commented Dec 6, 2022

For the record: I consider the sleep() function to be superior to the old resolve() and reject() functions, but after using this for a while, I think that (re-)using the name "sleep" was not necessarily the best idea. This has definitely been a source of confusion when reviewing code, plus IDE autocompletion may either offer PHP's built-in sleep() or the namespaced React\Promise\Timer\sleep() function, which use different argument types and return values and show different behavior.

I've just filed a new delay() function for reactphp/async in reactphp/async#69 which I believe to be a better name and default behavior considering our plans for ReactPHP v3 as shown in https://github.com/orgs/reactphp/discussions/481. There are plans to deprecate reactphp/promise-stream in the future as per https://github.com/orgs/reactphp/discussions/475 and I wonder if we should perhaps also aim to deprecate reactphp/promise-timer in the future given its limited use and suboptimal API design.

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

Successfully merging this pull request may close these issues.

2 participants