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

Promises #86

Closed
sagikazarmark opened this issue Nov 22, 2015 · 13 comments
Closed

Promises #86

sagikazarmark opened this issue Nov 22, 2015 · 13 comments
Milestone

Comments

@sagikazarmark
Copy link
Member

Implementations

Currently we have our own Promise interface, which is fine. It seems that we won't have a Promise PSR anytime soon.

However, there are already awesome promise packages:

I remember that Guzzle used to use ReactPHP's promise implementation. Maybe @mtdowling could tell us why they created their own?

Anyway: would it make sense to rely on third party promise implementations instead of ours?

Possible ways:

  • Keep the interface in the contract and use wrappers around third-party promises
  • Drop our interface and rely on a third-party directly (which would require relying on an implementation in the contract)

Convenience methods

Currently we have two convenience methods: getResponse and getException. These are what make our interface a "special" promise implementation. As the above two implementations, ours claim to be an implementation of Promises/A+ standard. Although I see the point in these methods, I think we should probably get rid of them.

Here are a few reasons:

  • If a Promise PSR ever becomes real, we would still need to extend it. In this case we couldn't just accept any kind of promises to be used.
  • The same case, if we want to use a third-party implementation now.
  • It is not part of the original standard, but as I can see, returning the value in the wait method is.

I propose removing those method. However in that case we need a "standard" way of getting the value and deciding whether it is an exception, a response, or an invalid value. Checking the type is obvious, but might led to duplicated code. Maybe we could check how Guzzle does it.

@sagikazarmark sagikazarmark modified the milestone: 1.0.0-beta Nov 22, 2015
@joelwurtz
Copy link
Member

I don't mind dropping the getResponse / getException method, problem is more in the return type of the onFulfilled callable or the throw type, that's must be fixed. Having those method makes, IMO, user / implementors understand more what the promise must return / use.

A valid promise specification allow anything for the value or for the reason. However in our case having something different from ResponseInterface / HttpException will break everything. (A plugin can return a string in the onCallable, this would be valid for the promise specification, but will break everything for the pluginChain)

ours claim to be an implementation of Promises/A+ standard

Not at all, but it may be not clear, it's an extension of this standard (basically it's the promise a+ standard with addition that the value and reason has a type: ResponseInterface / Http\Exception)

@sagikazarmark
Copy link
Member Author

Having those method makes (maybe) user / implementors understand more what the promise must return / use.

I would say we should clearly document this. That would probably be enough.

However in our case having something different from ResponseInterface / HttpException will break everything.

Promises are created "internally" by adapters/clients. Although the promise spec allows any values, we force it to be Response/HttpException. However the data itself is not resolved by the promise, the promise only holds it (as soon as the promise is resolved). So I think we don't need this restriction on an interface level.

As I said, I see the point in a custom interface, but I am afraid it could limit the usage (like switching to an upcoming PSR, using third-party, etc)

@mtdowling
Copy link

Guzzle has its own implementation to get rid of recursive promise resolution, add the ability to wait on a promise, add the ability to unwrap a promise, and to use Promises/A+ instead of Promises/A (dropping the progress callbacks).

@sagikazarmark
Copy link
Member Author

Thanks for clarification @mtdowling!

@sagikazarmark
Copy link
Member Author

After playing with this: we cannot use Guzzle or any other implementation if we stick to our interface.

So we either have to drop ours and stick to guzzle (don't like that) or maintain our own.

Even if we go with option two, the convenience methods should be removed as we couldn't rely on them anyway in case of using the upcoming PSR.

Thinking about PSR: In case we will have a PSR, we have to remove our own interface from the repository and require psr/promise. Why not doing it now? Move the promise interface into its own repository.

Pros:

  • Client Tools repos does not have to rely on httplug, only promise
  • We can easily implement promises without relying on httplug
  • Stick to the actual architecture we will eventually use, so when we switch to PSR, we don't have to touch httplug

Cons:

  • One more package

@dbu
Copy link
Contributor

dbu commented Nov 27, 2015 via email

@sagikazarmark
Copy link
Member Author

Yeah. If we magically end up with the same or very similar API as the PSR then we can implement it and deprecate the old without major version change.

@sagikazarmark
Copy link
Member Author

Whis one looks cool:

https://github.com/thenable/promise

@dbu
Copy link
Contributor

dbu commented Nov 27, 2015 via email

@sagikazarmark
Copy link
Member Author

Not really, it is similar. I am about to contact the author and ask how we could use this. It seems that this is going to be some sort of PSR proposal.

I don't insist on it, but would be cool to pass this responsibility to developers who work with promises already. 😛

@joelwurtz
Copy link
Member

We should keep it simple for the moment and stick to our current interface. If there is something to change we can always make adapter / major version if we keep something light.

There is a long way before having a promise PSR and really don't know where it will be going (for the moment they need a EventLoopInterface before considering the promise one)

Don't mind dropping the getException / getReponse method in favor of returning a response / throwing an exception in the wait method, from my view it doesn't change anything.

@sagikazarmark
Copy link
Member Author

Sounds like a plan.

@sagikazarmark
Copy link
Member Author

This has been solved by extracting the Promise interface to it's own package and relying on our own implementation.

Nyholm pushed a commit to Nyholm/httplug that referenced this issue Dec 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants