SignalCommandMap and IInjector

Maciek's Avatar

Maciek

07 Jul, 2010 06:59 PM

Recently I had to make the following change in my code:

-            signalCommandMap.mapSignalClass(MySignal, MyCommand);
+            signalCommandMap.mapSignal( injector.getInstance(MySignal), MyCommand);

I'm working with the SignalCommandMap in a modular manner: the main application has a set of signals, and there are any number of "module" instances that have their own child contexts. This works well, but until I hit this issue, I didn't realize that mapSignalClass instantiates a Signal even if it has been mapped by the associated IInjector elsewhere. This is exactly the case when using the SignalCommandMap in a modular manner (I expected to inherit the Signal mappings from my parent IInjector).

I think I see why this is done: so that you don't need to explicitly map signals for both the IInjector (for DI purposes) and the signal command map (for signal -> command purposes). However, it does cause somewhat unexpected behavior.

Is there a reason the SignalCommandMap does its own mapping, instead of letting the IInjector do this? It seems that signalClassMap and IInjector.instantiate could be dropped in favor of IInjector.getInstance, no? It seems the SignalCommandMap could ask IInjector for an instance and either (a) manage its own if none exists, or (b) create a mapping in the IInjector if none exists. I can see that (b) is somewhat dicey (it silently creates mappings for you that could have wider-ranging consequences), but (a) seems reasonable. Also, I think even (b) is exceedingly unlikely to cause problems in real-world usage.

Any thoughts?

  1. Support Staff 1 Posted by Joel Hooks on 07 Jul, 2010 07:07 PM

    Joel Hooks's Avatar

    I think you are correct, and if you wanted to update a fork with the functionality and any unit tests (new or updated) it could be pulled in. I'd appreciate it. I'm swamped.

  2. 2 Posted by Maciek on 07 Jul, 2010 07:12 PM

    Maciek's Avatar

    Ok, thanks for the feedback. I'm also swamped, but it's a small change, and I'll see if I can pull it off in my copious spare time. Any preference for behavior (a) or (b) above?

  3. Support Staff 3 Posted by Joel Hooks on 07 Jul, 2010 07:15 PM

    Joel Hooks's Avatar

    I agree with you that a is probably the sensible choice.

  4. 4 Posted by Maciek on 09 Jul, 2010 07:03 AM

    Maciek's Avatar

    Hmm. So I looked into this, and:

    private function createSignalClassInstance(signalClass:Class):ISignal
    {
        var injectorForSignalInstance:IInjector = injector;
        var signal:ISignal;
        if(injector.hasMapping(IInjector))
            injectorForSignalInstance = injector.getInstance(IInjector);
        signal = injectorForSignalInstance.instantiate( signalClass );
        injectorForSignalInstance.mapValue( signalClass, signal );
        signalClassMap[signalClass] = signal;
        return signal;
    }
    

    Which already seems to do a questionable version of (b) above. According to the docs, instantiate() followed by mapValue() will give the same behavior as registering a singleton and calling getInstance(), no? Is that mapValue() just there by mistake? I don't think it's necessary for the signal command map to work properly. Alternately, if it is intentional (and the map is supposed to create mappings in the injector when signals are mapped to commands), we can accomplish a proper (b) by just switching to mapSingleton() and getInstance(). Any thoughts?

  5. Stray closed this discussion on 13 Feb, 2011 04:27 PM.

Comments are currently closed for this discussion. You can start a new one.

Keyboard shortcuts

Generic

? Show this help
ESC Blurs the current field

Comment Form

r Focus the comment reply box
^ + ↩ Submit the comment

You can use Command ⌘ instead of Control ^ on Mac