chore(): Remove cache dependency and move others to dev dependencies #11
Conversation
|
|
||
| if (false === isset($container[self::REQUEST_FACTORY])) { | ||
| $container[self::REQUEST_FACTORY] = static function (Container $container) { | ||
| if (false === isset($container[self::REQUEST_FACTORY]) && class_exists('Nyholm\Psr7\Factory\Psr17Factory')) { |
There was a problem hiding this comment.
I'm not a big fan of service providers in libraries anymore, how about removing it and "replace" it with a "how to use" part in the readme? 😉
There was a problem hiding this comment.
So what do you do then? Just duplicating stuff? I was also thinking about the whole reusable service providers. In this case here we could make it configurable with service ids you can inject over the constructor so it can locate a request factory and a client and works for the schema api client... or do you just started duplicating the whole service provider logic over and over again?
There was a problem hiding this comment.
Basically, yes ^^ To me, service providers are application-specific glue code and therefore shouldn't be provided by libraries.
In the core project, we just made the experience that sooner or later the service providers provided by the libraries don't suit the application needs at some point, either because of different dependencies for the same interface, because of the introduction of either an adapter or a decorator or because you want to re-use an already registered service, etc. Another benefit imo is that if you don't use the provided service provider it's easier to see how the application is "glued" together. And on top of that, we could get rid of the additional dependencies in the libraries.
But I guess this is highly opinionated 🙂 . So I'm not saying we must get rid of the service providers in the libraries since everyone is free to not use them and I agree your solution provided with this PR is an improvement. Especially due to moving the "external" dependencies to the dev-dependencies 👍
So either way is fine to me, just quickly wanted to raise the thought. One suggestion I have, though, is that we should use PSR-11 instead of pimple, that way, it's at least container implementation agnostic.
|
@healerz You need to merge it now. 😉 |
|
@TiMESPLiNTER do I? 😆 |
No description provided.