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

Require discovery 0.9 and do not handle Puli. #81

Merged
merged 5 commits into from
Jun 29, 2016
Merged

Require discovery 0.9 and do not handle Puli. #81

merged 5 commits into from
Jun 29, 2016

Conversation

Nyholm
Copy link
Member

@Nyholm Nyholm commented Jun 28, 2016

I require discovery library, that is now okey since it is not a very light weight package. Since Puli is optional and handled in the discovery package we do not longer need to handle that in the bundle.

This will fix #80.

Tests are failing, we are waiting on php-http/buzz-adapter#3 and new patch versions on buzz-adapter and react-adapter

@sagikazarmark
Copy link
Member

Something is wrong with the composer.json

@sagikazarmark
Copy link
Member

Packages released

@Nyholm
Copy link
Member Author

Nyholm commented Jun 28, 2016

Thank you for the quick review and new releases. I've updated the PR

@dbu
Copy link
Collaborator

dbu commented Jun 29, 2016

looks like we have a problem with prefer-lowest. do you need to up one of the dependency further?

@dbu
Copy link
Collaborator

dbu commented Jun 29, 2016

thanks for the PR, great job, we can delete some code!

@Nyholm
Copy link
Member Author

Nyholm commented Jun 29, 2016

Yes, It always feels good creating a PR like this were you remove a lot of code and barley adding any.

Tests are passing now.

@dbu dbu merged commit b178535 into master Jun 29, 2016
@dbu dbu deleted the no-puli branch June 29, 2016 08:10
@sagikazarmark
Copy link
Member

Do we really need the discovery as a dependency? Or check for it if the classes are set in the config, and complain only in that case? Like we did with puli?

@Nyholm
Copy link
Member Author

Nyholm commented Jun 29, 2016

That is a good question Mark. As far as I can remember, we did not require Puli by default to give the user a change to use the bundle without Puli. That was because Puli was a to large of dependency.

Since version 0.9 our discovery layer is very light weight and I think it is okey to require discovery by default. This will give a better developer experience if things work out of the box.

@sagikazarmark
Copy link
Member

My only concern is that discovery is not stable yet, so version conflicts are more likely to happen.

@Nyholm
Copy link
Member Author

Nyholm commented Jun 29, 2016

Im not that concerned. As long as we are really careful with following semver.

@dbu
Copy link
Collaborator

dbu commented Jun 29, 2016 via email

@sagikazarmark
Copy link
Member

what is missing to make discovery stable, btw?

Well, now that Puli is not a requirement anymore, probably nothing. But if Puli breaks BC in the stable version, we have a problem.

@Nyholm
Copy link
Member Author

Nyholm commented Jun 29, 2016

I created an issue for this on the discovery repo. php-http/discovery#62

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.

Remove Puli dependency
3 participants