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

Introduce Gate service #12

Open
Biont opened this issue Sep 2, 2022 · 6 comments
Open

Introduce Gate service #12

Biont opened this issue Sep 2, 2022 · 6 comments

Comments

@Biont
Copy link
Collaborator

Biont commented Sep 2, 2022

Hello everyone.

In my projects, it is quite common to use a generic service ID to delegate to one of many specific services based on some context.

'app.view' => new Factory([ 
    'app.view.frontend',
    'app.view.backend',
    'app.is-frontend',
    ],
    function( View $frontend, View  $backend, bool $isFrontend){
        return $isFrontend ? $frontend : $backend;
    }
)

While this is workable, it is not as concise as it could be and -worse- it needs to create all instances before making the descision which one to actually return.
I think at least for the A/B scenario this could be greatly improved by the introduction of a simple class Gate( string $service1, string $service2, callable $predicate ):

'app.view' => new Gate( 
    'app.view.frontend',
    'app.view.backend',
    function( ContainerInterface $c ){
        return (bool) $c->get('app.is-frontend')
    }
)

This service would call the $predicate function and grab either the first or the second ID from the container based on the result.

Spinning this a bit further, $predicate could also be string|callable so that it optionally accepts another service ID pointing at the actual predicate (which is expected to return bool)
Then you could write extremely compact Gate definitions like this.

'app.view' => new Gate( 
    'app.view.frontend',
    'app.view.backend',
    'app.is-frontend'
)

Turning this idea around, even the services themselves could be string|callable for prototyping service definitions really quickly:

'app.view' => new Gate( 
    fn() => new Frontend(),
    fn() => new Backend(),
    'app.is-frontend'
)

And this would also allow nesting Gates for staggeringly flexible and powerful definitions:

'app.view' => new Gate( 
    new Gate(
        'app.view.cart',
        'app.view.catalog',
        'app.is-cart'
    ),
    'app.view.backend',
    'app.is-frontend'
)

What do you think about this idea? Would you accept a PR for this?

@mecha
Copy link
Member

mecha commented Sep 12, 2022

+1 from me. Mainly for this reason:

it needs to create all instances before making the decision which one to actually return

I've encountered cases where a service is expensive to create, and due to how it gets wired into the application, it would get created even when it is not necessary.

Personally though, I'd vote to have the predicate be the first argument so that the structure is similar to that of a ternary expression. I find that to be more intuitive.

'app.view' => new Gate(
    'app.is-frontend',
    'app.view.frontend',
    'app.view.backend'
)

Regarding nesting: this has been on my mind a number of times, although not nesting itself, but its "superset" of allowing arbitrary values and service hierarchies to be passed as dependencies.

For instance, a common thing that I find myself doing is using the Constructor service and wanting to pass a scalar as an argument, such as a boolean flag. The Constructor service helper only accepts strings IDs, so to pass a scalar I'd need to create a separate service for it, and then pass its ID to the Constructor.

I have a couple of solutions in the works that would allow services to work with anything, not just service IDs. So for now I'd be hesitant to add a one-off for the Gate, but I wouldn't be outright opposed to it.

@XedinUnknown
Copy link
Member

Heya! Sorry for delay. Will look into this soon!

Good to see you, guys :)

@XedinUnknown
Copy link
Member

Here are my thoughts on this.

  1. First of all, while there seem to be cases where this it at least appears beneficial to do this, it is not really in line with my vision for configuration. Which is: the application's dependency graph should only change when the application level (modules) change, and not when, say, some environment variable, like request data, changes. Use-cases like A/B testing are very valid, but I'm not sure the conditions it is concerned with should have an effect on the dependency graph. If they do, it may lead to unpredictable consequences due to some services not loading etc.
  2. Seems like a pretty simple thing to have a re-usable solution for. Granted, though, if it saves a bit of time and human error every time, it may be worth doing anyway.
  3. In conjunction with 2, perhaps this can be made more generic in order to give more value to the re-usable part. For example, since false is also a result of (bool) 0 and true is a result of (bool) 1, perhaps the value returned can be any non-negative int, and that determines which of the arguments (starting from 0) to return. For 2 arguments, it should work exactly like what you have, but permitting more than just 2 values.
  4. I also like the idea of putting the predicate first. It would allow for a variadic signature for more than 2 choices, if necessary, which in conjunction with 3 is a nice bonus.
  5. The idea of accepting instances/values other than service IDs is very interesting. There are classes here already that exist to provide the ability to do that, while keeping the underlying config standard. Seems like a good thing to consider doing, regardless of whether we implement Gate.

@XedinUnknown
Copy link
Member

By the way, just found this interesting NEON format, which is excellent for service definitions thanks to entities feature.

@mecha
Copy link
Member

mecha commented Nov 24, 2022

the application's dependency graph should only change when the application level (modules) change, and not when, say, some environment variable, like request data, changes

If you don't want your dependency graph to change under those situations, then don't wire your services that way. But imposing that vision into this package's design is, in my opinion, unhealthy for a package whose sole purpose is to offer an alternative to plain closures.

We should aim to provide options to consumers, with a focus on ease of use, clean code, and low friction when writing services. Configs, NEON, standards, and personal vision should have their own place; this package ain't it.


On that same note, I'll be opening a couple of issues shortly that address some of the pain points I've found after using this package in a commercial product for over 2 years.

For this issue, I think we should offer a Gate implementation, but keep it simple for now. We can address the other quality-of-life features in those separate issues.

@mecha
Copy link
Member

mecha commented Nov 29, 2022

@Biont You might want to take a look at #14 and its corresponding PR to see my take on flexible service dependencies, which is similar to what you proposed for Gate.

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

No branches or pull requests

3 participants