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

Common Driver Implementations #151

Open
trowski opened this issue Feb 22, 2017 · 12 comments
Open

Common Driver Implementations #151

trowski opened this issue Feb 22, 2017 · 12 comments

Comments

@trowski
Copy link
Contributor

trowski commented Feb 22, 2017

The Driver interface leaves no room for differing behaviors between implementations. Once a particular vendor has written performant Driver implementations for various backends there is little reason for another vendor to do the same. Changes in behavior between Driver implementations render them non-interoperable. Perhaps then this group should also offer Driver implementations, dissociating Driver implementations from any particular vendor.

amphp/loop contains highly optimized Driver implementations based on the native stream_select(), libevent (ext-event), libev (ext-ev), and libuv (ext-uv). Should this library be migrated to be a part of this project?

@kelunik
Copy link
Member

kelunik commented Feb 22, 2017

Huge 👍, makes using the loop way easier. People can just require async-interop/event-loop and things work. I'd still keep the factory mechanism, so people can still write their own implementations, maybe with custom underlying extensions.

@joshdifabio
Copy link
Contributor

joshdifabio commented Feb 22, 2017

What if we ended up with two versions of the uv driver which worked with different versions of the uv extension? Perhaps the drivers which rely on extensions should be packaged separately and we should just include a stream_select loop?

Edit: I suppose in that case we could instead have two uv drivers in this package, with the correct one picked based on the extension version. Or the driver could simply be designed to switch behaviour based on the extension version. Maybe this isn't an issue.

@kelunik
Copy link
Member

kelunik commented Feb 22, 2017

@joshdifabio This package will take care of it then and add a new driver, just like any other package would. Not including the extension based drivers is a no-go IMO, as it effectively doesn't bring any value then.

@joshdifabio
Copy link
Contributor

joshdifabio commented Feb 22, 2017

This would certainly simplify usage. It does raise the question of what the difference between this package and, say, amphp/loop would actually be. Clearly replacing vendor-specific loop packages would be the intention here but I wonder if React and others would actually be on board with this.

@kelunik
Copy link
Member

kelunik commented Feb 22, 2017

@joshdifabio React adapting the interop loop is highly unlikely. Everything we can do is provide an adapter so every interop library / application can use any React library.

It does raise the question of what the difference between this package and, say, amphp/loop would actually be.

None, they're just merged then, it's the exact proposal.

@joshdifabio
Copy link
Contributor

Okay. I think I'm on board as well.

@trowski
Copy link
Contributor Author

trowski commented Feb 22, 2017

@sagebind
Copy link

I'm in favor. Only issue I see is from a community perspective it might seem like a conflict of interest. It might send the wrong idea that implementing the standard is discouraged, since we would point to using our implementation instead.

@kelunik
Copy link
Member

kelunik commented Feb 25, 2017

@sagebind Implementing the already supported backend extensions again is indeed unnecessary. If there's room for improvement, submit a PR to this repository instead. Custom implementations will be allowed, but not the usual way of doing things.

@kelunik
Copy link
Member

kelunik commented Feb 25, 2017

Also ping @cboden and @clue.

@rdlowrey
Copy link

rdlowrey commented Mar 2, 2017

👍 ... particularly if including a react adapter is a feasible option

@trowski
Copy link
Contributor Author

trowski commented Mar 2, 2017

@rdlowrey I wouldn't include it in this package, but having a separate package under async-interop for a React adapter would be a possibility.

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

No branches or pull requests

5 participants