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

adapt for amphp promises #100

Closed
wants to merge 1 commit into from
Closed

adapt for amphp promises #100

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Sep 9, 2017

@cboden
Copy link
Member

cboden commented Sep 9, 2017

I think this type of support would be better in its own repo. I don't like the idea of introducing an API that relies on a library that may or may not be available.

@ghost
Copy link
Author

ghost commented Sep 9, 2017

https://github.com/umbri/amp-react-interop

@ghost
Copy link
Author

ghost commented Sep 9, 2017

Just in case: I am using namespace React\Promise
https://github.com/umbri/amp-react-interop/blob/master/lib/functions.php#L33

Should I change that ?

@cboden
Copy link
Member

cboden commented Sep 9, 2017

Yes please. React is this organizations namespace.

@ghost
Copy link
Author

ghost commented Sep 9, 2017

Ok, no problem I will change that

@kelunik
Copy link

kelunik commented Sep 10, 2017

@cboden While a separate package could also make sense, I think including something like that into another package like this one directly can make sense for improved discoverability.

The type declaration could also be dropped in the future to allow for other promise implementations to be easily adapted, too.

@kelunik
Copy link

kelunik commented Sep 10, 2017

@umbri I think it would make sense to simply support it in here instead of a separate adapt method.

@jsor
Copy link
Member

jsor commented Sep 10, 2017

I think it makes perfect sense to provide that via a separate package. If Amp wants to be consumable by promise libs, it can provide a then() method. This would make it consumable by ReactPHP's promises and also Guzzle's promises.

@clue
Copy link
Member

clue commented Sep 10, 2017

Thanks for the PR @umbri, I think this sparked a very good discussion and lead the right thing: Your adapter is much appreciated and I suppose something that people that are interested in using amphp with ReactPHP would be interested in 👍

Other than that, I agree with @cboden and @jsor in that it doesn't make much sense to provide adapters for all kinds of (niche) third party projects here, in particular given that we already support "foreign promises" (anything with a standard then() method).

@kelunik As much as I understand your effort, I don't think this project should be concerned with "improved discoverability" of (niche) third party projects that this project has no involvement whatsoever with.

I believe this has been answered, so I'm closing this for now. Please come back with more details if this persists and we can reopen this 👍

Again, thanks for the discussion everybody and thanks @umbri for bringing this idea to the table 👍

@clue clue closed this Sep 10, 2017
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

Successfully merging this pull request may close these issues.

4 participants