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

Merge promise interfaces #75

Merged
merged 4 commits into from
Jan 4, 2017
Merged

Merge promise interfaces #75

merged 4 commits into from
Jan 4, 2017

Conversation

jsor
Copy link
Member

@jsor jsor commented Dec 21, 2016

Merge PromiseInterface, ExtendedPromiseInterface and CancellablePromiseInterface into a single PromiseInterface.

The CancellablePromiseInterface is kept for backward compatibility but is marked as deprecated and must not be used anymore. (Edit: Also removed in 251b4e5)

Closes #44

Merge PromiseInterface, ExtendedPromiseInterface and
CancellablePromiseInterface into a single PromiseInterface.

The CancellablePromiseInterface is kept for backward compatibility
but is marked as deprecated and must not be used anymore.

Closes #44
@jsor jsor requested a review from clue December 21, 2016 19:16
Copy link
Member

@WyriHaximus WyriHaximus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So much 👍 on this, type hinting is a P.I.T.A. if you want to be compatible and ensure the promise can be canceled

@jsor
Copy link
Member Author

jsor commented Dec 22, 2016

I mostly kept CancellablePromiseInterface for @clue because i've seen him using instanceof checks a lot in his libs, eg: https://github.com/reactphp/promise-timer/blob/master/src/functions.php#L23 😃

@kelunik
Copy link

kelunik commented Dec 23, 2016

Are you sure merging CancellablePromiseInterface is a good idea? Most operations can't be canceled I guess.

@jsor
Copy link
Member Author

jsor commented Dec 23, 2016

@kelunik All promises have a cancel method. If calling this method has any effect is up to the producer of the promise.

@kelunik
Copy link

kelunik commented Dec 23, 2016

@jsor Might make sense then as long as cancel is don't care then instead of abort.

@jsor
Copy link
Member Author

jsor commented Dec 23, 2016

I'm not sure i understand what that has to do with the interface merge? The interfaces are only separate for backward compatibility reasons because they were introduced separately. All promise implementations in react/promise implement all 3 interfaces.

@kelunik
Copy link

kelunik commented Dec 23, 2016

Ah, fine then. I thought cancelability was an optional thing before.

@jsor
Copy link
Member Author

jsor commented Jan 4, 2017

I've now also removed CancellablePromiseInterface.

When supporting react/promise ~1.2|~2.1|~3.0 as a dependency, the following code is not longer possible:

if ($promise instanceof CancellablePromiseInterface) {
    $promise->cancel();
}

A possible solution is to pass $promise through resolve().

React\Promise\resolve($promise)->cancel();

Please note, that this adds some overhead if $promise is not already a promise instance, eg. a scalar.

Another solution would be to just test against PromiseInterface.

if ($promise instanceof PromiseInterface) {
    $promise->cancel();
}

All promise implementations from react/promise < 3.0 implement both PromiseInterface and CancellablePromiseInterface, so every instance of PromiseInterface is also an instance of CancellablePromiseInterface.

This would break though, when $promise is a custom third-party promise implementation which only implements PromiseInterface but not CancellablePromiseInterface.

The safest solution would probably be a combination of the two methods:

if ($promise instanceof PromiseInterface) {
    React\Promise\resolve($promise)->cancel();
}

@jsor jsor requested a review from WyriHaximus January 4, 2017 13:47
Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! :shipit: The changes make perfect sense and the upgrade guide is much appreciated! 👍

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

Successfully merging this pull request may close these issues.

4 participants