async-interop / event-loop

An event loop interface for interoperability in PHP.
MIT License
170 stars 9 forks source link

Composer: Add API virtual package and require an implementation #136

Closed joshdifabio closed 7 years ago

joshdifabio commented 7 years ago

Resolves #129.

There's a potential issue here: Travis is now going to require a compatible event-loop-implementation in order to run the tests for event-loop. To solve this, perhaps we could create an event-loop-dummy-implementation repo in async-interop, not add it to Packagist so it's not pulled in elsewhere, move tests\DummyDriver.php to that new repo, and add the following to the event-loop composer.json file:

{
    "repositories": [ { "type": "git", "url": "https://github.com/async-interop/event-loop-dummy-implementation.git" } ]
}

Thoughts?

kelunik commented 7 years ago

We could also move async-interop/event-loop-implementation into suggest.

joshdifabio commented 7 years ago

We could also move async-interop/event-loop-implementation into suggest.

We'd then have the issue of people having to specify dual requirements (api and implementation) in their libs.

kelunik commented 7 years ago

I don't think people really need to specify the event-loop-implementation requirement.

joshdifabio commented 7 years ago

I don't think people really need to specify the event-loop-implementation requirement.

Personally, I agree. I think the implementation requirement will only be needed in certain packages. I think if we're all happy with that then it would make sense to move event-loop-implementation to suggest.

@bwoebi @trowski

kelunik commented 7 years ago

will only be needed in certain packages

Which ones?

joshdifabio commented 7 years ago

Which ones?

Ones like Guzzle which might use a loop but hide that fact from applications.

kelunik commented 7 years ago

We don't care about such packages here. Every package that hides the loop isn't interoperable.

joshdifabio commented 7 years ago

When I say hide the loop, I mean by implementing functionality which implicitly runs the loop and awaits promise resolution instead of requiring clients to bootstrap and run the loop themselves, such as how Guzzle does with its awaitable promises. This is useful in synchronous applications.

joshdifabio commented 7 years ago

Anyway, I'm happy to make this a suggest if everyone else is.

kelunik commented 7 years ago

@joshdifabio I know what you mean and IMO this is not implementable in a interop way, because it blocks then.

trowski commented 7 years ago

Moving event-loop-implementation to suggest is an acceptable solution IMO. I would hope most people would quickly figure out their problem if the suggestion popped up.

joshdifabio commented 7 years ago

Okay, I'm gonna move it to suggest.

joshdifabio commented 7 years ago

I should squash commits before this gets merged.

kelunik commented 7 years ago

@joshdifabio What's still missing is a note in the README about libraries only depending on event-loop-api and only driver implementations depending on event-loop directly.

joshdifabio commented 7 years ago

What's still missing is a note in the README about libraries only depending on event-loop-api and only driver implementations depending on event-loop directly.

Right. Is someone working on a proper readme & metadoc?

kelunik commented 7 years ago

@bwoebi Wanted to do that, don't know whether he started yet. But in the meantime, a note in the README is fine I guess.

kelunik commented 7 years ago

I just noticed that the suggest doesn't make any sense if packages only depend on the event-loop-api, because event-loop will only be pulled as dependency of the driver implementation.

kelunik commented 7 years ago

While we might want to add a new method somewhere in the future, I think the proposed ways are too complicated and aren't worth the trouble, we should just increase the major version in case we need to add a method. I think it's unlikely to happen after 1.0 is out.

kelunik commented 7 years ago

I think we should just go with #151 and then we don't need anything like this.

joshdifabio commented 7 years ago

I think we should just go with #151 and then we don't need anything like this.

As long as we allow others to define their own drivers #151 doesn't actually change anything. We would still need to provide some kind of semantic version guarantee to both loop users and loop implementors. The question is still whether we would want to bump the major version for loop users when we change the driver in a way which only breaks implementations.

I don't think this PR represents any real complexity, but it gives loop users the power to opt-in to non-breaking API changes to the driver interface. I think that's worth the single provide in composer.json.

kelunik commented 7 years ago

I don't think this PR represents any real complexity, but it gives loop users the power to opt-in to non-breaking API changes to the driver interface. I think that's worth the single provide in composer.json.

What's the proposal for libraries to depend on after #151 is merged?

joshdifabio commented 7 years ago

What's the proposal for libraries to depend on after #151 is merged?

If I understand the question, I see three options:

  1. We have no virtual packages and only maintain a single version number. The version number covers the SPI and so any changes to Driver would be a major version bump for Loop clients as well as Driver implementors.
  2. We have a virtual package async-interop/event-loop-api which covers the client API, while the main version number covers the SPI. Libraries which don't implement or extend Driver would depend on async-interop/event-loop-api. Libraries which implement or extend Driver would depend on async-interop/event-loop.
  3. We have a virtual package async-interop/event-loop-spi which covers the SPI, while the main version number covers the client API only. Libraries which don't implement or extend Driver would depend on async-interop/event-loop. Libraries which implement or extend Driver would depend on async-interop/event-loop-spi.

I think 3 would be my preferred approach.

kelunik commented 7 years ago

Closing, as project discontinued for now.