search

maknz / slack

A simple PHP package for sending messages to Slack, with a focus on ease of use and elegant syntax.
BSD 2-Clause "Simplified" License
1.17k stars 204 forks source link

Defer service provider registration #26

Closed brunnoferreira closed 9 years ago

brunnoferreira commented 9 years ago

@maknz,

Wouldn't deferring the registration by setting $defer = true be a better default for the project? This seems to be the default behavior for most third-party providers and allows for better app performance.

As an example, we've been using your package on a big project, but only part of it should actually integrate with Slack (we're using it for debugging purposes). Most of our users won't even have the ability to trigger the Slack integration, but the service provider is being loaded nonetheless.

Thanks for your time and for a great package.

maknz commented 9 years ago

Makes sense. I'll implement and test that tonight.

Glad you like the package :-)

On Tue, Jun 16, 2015 at 5:51 PM, brunnoferreira notifications@github.com wrote:

@maknz, Wouldn't deferring the registration by setting $defer = true be a better default for the project? This seems to be the default behavior for most third-party providers and allows for better app performance. As an example, we've been using your package on a big project, but only part of it should actually integrate with Slack (we're using it for debugging purposes). Most of our users won't even have the ability to trigger the Slack integration, but the service provider is being loaded nonetheless.

Thanks for your time and for a great package.

Reply to this email directly or view it on GitHub: https://github.com/maknz/slack/issues/26

maknz commented 9 years ago

Right, so after a bit of research, since the service providers provide configuration, they cannot be deferred. I've had a look at a few Laravel packages that do both configuration and service container bindings, and none of them are deferred (e.g. intervention/image and graham-campbell/flysystem). Could you point me in the direction of packages that overcome this issue (i.e. that defer bindings but also offer configuration?)

To theorise, the cost of instantiating a client to the service container should be negligable. Unless Guzzle is reasonably heavy -- do you have any benchmarks?

brunnoferreira commented 9 years ago

I don't, actually. We have just started using your Slack package, and it integrates with a minor part of our system, so any benchmarks so far are nearly useless.

It was more of a philosophical input, rather than overcoming a known issue. I was just looking for the best way to handle its usage, but failed to notice that there's some configuration being done apart from the bindings.

This definitely makes it inappropriate for deferring. So sorry for the mistake and for taking up some of your time. I'll go ahead and close this issue.

maknz commented 9 years ago

No worries, thanks for brining it up :-)

BTW, if efficiency is uber-important, you can duplicate the service provider to your app namespace and defer it there, removing the config registration code and just have the service bindings. If you manually put the config file in the right place, the service provider doesn't need to do that -- it's just so you can use vendor publish commands to automatically find the config from the package and publish it out to the app.