ssannandeji / Zenject-2019

Dependency Injection Framework for Unity3D
MIT License
2.53k stars 363 forks source link

Signal declaration withId outside installers #587

Closed JJCaballeroSt closed 5 years ago

JJCaballeroSt commented 5 years ago

Hi! Im totally new into this kind of injection in Unity, and I'm having a lot of trouble making a signal system working when the declaration it's not in an installer.

What I'm trying to do it's to filter dynamically when the objects are created who will receive the signals with the ID option.

The code is this:

    [Inject] SignalBus _signalBus;

    [Inject] private DiContainer container;

    // Start is called before the first frame update
    void Start()
    {
        container.DeclareSignal<ViewSignalObj>().WithId("whatever");

        _signalBus.Fire<ViewSignalObj>("whatever");
    }

But it only throws an exception saying that the signal with that type and the "whatever" string doesnt exist...

I'm I doing something wrong or it's that the signal system can only be declared on the installers and not dynamically?

JJCaballeroSt commented 5 years ago

I have studied the code and I think that the problem is that, since the list of signals it's added only when the SignalBus class it's created, any signal declared after that won't be added.

For example, this code works just fine and adds the "fooDerp" signal into the bus

        /*
         * Installer that installs the SignalBus and adds a couple of signals
         */
        MonoInstalador.InstallFromResource(container);

        container.DeclareSignal<ViewSignalObj>().WithId("fooDerp");

        var signalBus = container.Resolve<SignalBus>();

BUT, if I move the signal declaration under the resolution of the signalBus, then it won't be added, because it has already been created.

        /*
         * Installer that installs the SignalBus and adds a couple of signals
         */
        MonoInstalador.InstallFromResource(container);

        var signalBus = container.Resolve<SignalBus>();
         // this won't add the signal into the SignalBus, only 
        // into the container list of declarations.
        container.DeclareSignal<ViewSignalObj>().WithId("fooDerp");

Calling a rebind won't do because if the SignalBus is already declared, then will throw an error as if I was creating a second one. Also, deleting all the binds in the container manually and declaring it again won't do (and it's pretty messy).

I think that the only option to allow (if that's an option that the framework wants to provide) dynamic signals after the declaration of the SignalBus, is to change the DeclareSignal and make it inject the new signals into the bus in case that is already declared?

svermeulen commented 5 years ago

I'm not opposed to allowing adding support for adding signal declarations at runtime. But I would suggest that we add a method to SignalBus for this, rather than expect people to use container.DeclareSignal. It's bad practice to add bindings to DiContainer after the install phase has completed. After install, you should only ever do resolve, instantiate or inject using DiContainer.

Would something like _signalBus.AddDeclaration<ViewSignalObject>("fooDerp") work for your case? (assuming we add that method)

JJCaballeroSt commented 5 years ago

Yeah, that would be perfect!

svermeulen commented 5 years ago

Ok I added that to master branch

JJCaballeroSt commented 5 years ago

Thank you!