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

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

Closed

Conversation

joshdifabio
Copy link
Contributor

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
Copy link
Member

kelunik commented Jan 8, 2017

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

@joshdifabio
Copy link
Contributor Author

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
Copy link
Member

kelunik commented Jan 8, 2017

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

@joshdifabio
Copy link
Contributor Author

joshdifabio commented Jan 8, 2017

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
Copy link
Member

kelunik commented Jan 8, 2017

will only be needed in certain packages

Which ones?

@joshdifabio
Copy link
Contributor Author

Which ones?

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

@kelunik
Copy link
Member

kelunik commented Jan 8, 2017

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

@joshdifabio
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@kelunik
Copy link
Member

kelunik commented Jan 8, 2017

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

@trowski
Copy link
Contributor

trowski commented Jan 8, 2017

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
Copy link
Contributor Author

Okay, I'm gonna move it to suggest.

@@ -15,6 +14,9 @@
"provide": {
"async-interop/event-loop-api": "0.1"
},
"suggest": {
"async-interop/event-loop-implementation": "A loop implementation is needed in order to run the loop."
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: "A loop implementation is required in order to run a loop."

@joshdifabio
Copy link
Contributor Author

I should squash commits before this gets merged.

@kelunik
Copy link
Member

kelunik commented Jan 9, 2017

@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
Copy link
Contributor Author

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
Copy link
Member

kelunik commented Jan 9, 2017

@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
Copy link
Member

kelunik commented Jan 19, 2017

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
Copy link
Member

kelunik commented Feb 17, 2017

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
Copy link
Member

kelunik commented Feb 22, 2017

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

@joshdifabio
Copy link
Contributor Author

joshdifabio commented Feb 22, 2017

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
Copy link
Member

kelunik commented Feb 22, 2017

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
Copy link
Contributor Author

joshdifabio commented Feb 22, 2017

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
Copy link
Member

kelunik commented Apr 17, 2017

Closing, as project discontinued for now.

@kelunik kelunik closed this Apr 17, 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