RL2 Gotcha

[Alec McEachran]

Hi guys and gals,

I have just dived back into the AS3 pool and so naturally have started porting the UI of the game I'm taking on over to RobotLegs2. I was coding fast, and injected

[Inject]

public var mediatorMap:MediatorMap;

into my Config. Everything worked as expected, the API was fine, but there was no automatic mediation. Only when I injected the interface

[Inject]

public var mediatorMap:IMediatorMap;

did I get the automatic mediation. Apart from my relative surprise that RL2 didn't shout at me for injecting MediatorMap directly, it led me to think about two things. Firstly, (and I realize there's another forum on this for which I've slightly missed the boat) are you really persevering with the last bastion of Hungarian notation, the IInterface? Seems very old-fashioned to me. Secondly, for me the MediatorMap/IMediatorMap confusion is friction that users should be made aware of through wikis or documentation. It took me a long time to realize that I should try with an I.

Lastly, just wanted to say that I'm very excited to be using RL2. I'm glad the seemingly endless missteps by Adobe hasn't killed this effort. Thanks.

[Neil Manuell]

on the IClaudius issue:

https://github.com/robotlegs/robotlegs-framework/issues/43 

just for completeness

--
You received this message because you are subscribed to the Google
Groups "Robotlegs" group.
To post to this group, send email to robo...@googlegroups.com
To unsubscribe from this group, send email to
robotlegs+...@googlegroups.com
for support visit http://knowledge.robotlegs.org

[Stray]

Alec: thanks for the head's up - I've added a line to the ReadMe for the MediatorMap, and we'll probably need a similar line in most utils - do you want to raise an issue on github for that? (I like to keep proper attribution where possible, for the grandkids etc).

Here's what's worrying me: did you not get an error? Injecting something that isn't available should puke pretty hard - silent fail is *not* what we were hoping for! If you can confirm that it's a silent fail, could you also raise an issue for that - initially in Robotlegs, and we can transfer it to Swiftsuspenders if necessary. (If you confirm it and can be bothered to write a failing test for it, even better!)

As Neil says, we've explored the IClaudius issue extensively. There are pros and cons to the IFace, and the pros won out, which doesn't negate your cons, just, on balance, we preferred to stick with the I. I'll leave you to read the thread if you can spare the hours ;)

I'm glad RL2 is here as well!

Sx

[Till Schneidereit]

> Here's what's worrying me: did you not get an error? Injecting something
> that isn't available should puke pretty hard - silent fail is *not* what we
> were hoping for! If you can confirm that it's a silent fail, could you also
> raise an issue for that - initially in Robotlegs, and we can transfer it to
> Swiftsuspenders if necessary. (If you confirm it and can be bothered to
> write a failing test for it, even better!)

Since https://github.com/tschneidereit/SwiftSuspenders/issues/36 got
fixed, you won't get an error for that anymore. This is a very good
example for why that change might not have been for the best, after
all. I'm still a bit on the fence, to be honest. (But see below.)

>
> As Neil says, we've explored the IClaudius issue extensively. There are pros
> and cons to the IFace, and the pros won out, which doesn't negate your cons,
> just, on balance, we preferred to stick with the I. I'll leave you to read
> the thread if you can spare the hours ;)

On the other hand, this same issue is yet another reason to be
anti-IClaudius. I still _really_ think that we should drop that, for
all the reasons I mentioned in that issue's discussion thread.

[Stray]

Till - your logic is normally beautiful, but I think that's a conflation of a general problem and a specific use case that has a specific fix.

The fact that the fix for issue 36 caused a problem in this case doesn't mean that if we get rid of the IClaudius the whole "silent fail - wtf?" issue would be resolved.

I won't expand on my support for IClaudius here because it's well covered on that thread :)

I'd prefer a different solution to issue 36, but I'll stick my comments on there - I missed it at the time, sorry. (And my thinking may be flawed - but we can explore that there).

Sx

[Till Schneidereit]

Stray, I definitely didn't want to imply that all problems with
Swiftsuspenders issue 36 (and its current solution) would go away with
IClaudius. That would be an incorrect conflation of two issues indeed.

I do however think that this specific issue exposes yet another good,
and even practical, reason for getting rid of the "I" prefix. Even if
Swiftsuspenders changes (which it very well might and I'm looking
forward to your proposal, Stray), getting an error at runtime is still
strictly worse than being pretty much forced to inject the right thing
because there just isn't anything else.

And now I have said too much about IClaudius again, sorry for that.


cheers,
till

ps: Neil, thanks for the "IClaudius" moniker - I love it!

[Neil Manuell]

Ha, was a toss up between IClaudius and IRobot :D

[Avi Kessner]

IRobot would have been better given the context, but IClaudius makes a better meme :)

Question, because I never tried it.  Does actionscript allow you to have an Interface with the same name as a Class?

brought to you by the letters A, V, and I
and the number 47

[Neil Manuell]

sure, as long as its in a different package

[Stray]

Can we agree that, to facilitate the DSL, there will always be a need for a separate interface and implementation? 

(MediatorMap implements more than one interface, the second of which is one that we don't want to expose to the user at all.)

[Till Schneidereit]

On Thu, Jul 12, 2012 at 1:49 PM, Stray <dailys...@googlemail.com> wrote:
> Can we agree that, to facilitate the DSL, there will always be a need for a
> separate interface and implementation?
>
> (MediatorMap implements more than one interface, the second of which is one
> that we don't want to expose to the user at all.)

Concrete classes can implement as many interfaces as the use-case
requires, of course. But I (still) don't really see why MediatorMap
should implement IMediatorMap, which is almost exactly a one-to-one
mapping of MediatorMap's public interface. In Swiftsuspenders, I have
InjectionMapping implement two interfaces for proper DSLing. I can't
see a single reason for having it implement a third one, simply
repeating its entire interface.

I know that we've been around this block several times already and it
seems as though, while I'm still not convinced of the case *for*
IClaudius (both in name and bare existence), you and others are still
not convinced of the case *against* it. Barring any new arguments,
that probably indicates we should leave things as-is.

A strongly related, but slightly different point is this: one of the
big points for IClaudius was that it makes it clear what you can
instantiate and what you can only work with. This seems to indicate
that there shouldn't be a publicly available MediatorMap at all
because you should *never* instantiate that yourself.

[joel...@gmail.com]

I've had a lot of success on a major project that wouldn't allow the
addition of a "framework" by simply extracting MediatorMap and
applying it to the project.

Not that this is helpful. Just that *never* is a very long time ;)

[Till Schneidereit]

hehe.

Just to be clear, I didn't intend to argue for a virtual
implementation that the computer draws directly out of your thoughts
or something ;)

Maybe the implementation should be package internal?

[joel...@gmail.com]

That is some REAL framework magic! Black box voodoo.

On Thu, Jul 12, 2012 at 8:47 AM, Till Schneidereit

[Till Schneidereit]

I'll say! To think I once considered IoC containers to be pretty much
as magic as it gets ...

[Stray]

Darn, this got long. Sorry.

> This seems to indicate
> that there shouldn't be a publicly available MediatorMap at all
> because you should *never* instantiate that yourself.

Agreed - and we already agreed *somewhere* that we would ship RL2 with a strong health warning that classes in the impl packages are for the RL2 framework's consumption only - you should only access API / DSL packaged classes.

Unfortunately if we put them in a proper 'restricted' package a la PMD then their usage inside the extensions themselves would also flag up as 'illegal' - which it isn't.

This is also why it can't be packaged internally - because it has to be available for extensions (one extension might rely on other maps). We did talk about custom namespaces as well, and decided that it wasn't ideal - perhaps we need to revisit that, but I think Joel's use case shows that it's overkill.

So the 99% rule is that unless you are an extension developer you should only import things in the API and DSL packages.

Alec is a little ahead of the game, so I'm not sure we have documentation spelling that out yet, or even whether we've done our final pass through the classes to check which package each belongs in. In *most* cases those classes will have (currently) an IClaudius look to them. The exceptions (other than events and errors) being classes such as Mediator which are intended for you to extend. (Again I *like* that clarity: inject against IClaudius, but don't try to extend IClaudiuses!)

> Concrete classes can implement as many interfaces as the use-case
> requires, of course. But I (still) don't really see why MediatorMap
> should implement IMediatorMap, which is almost exactly a one-to-one
> mapping of MediatorMap's public interface.

I think it's actually so simple that you're looking for something bigger.

The mediatorMap also implements IViewHandler.

The IViewHandler methods (in this case the handleView method) are not for public consumption. Their presence in the api is confusing. They are not repeated in the IMediatorMap interface (your comment about repeating in a 3rd interface confused me as we're not doing that here).

The only way to restrict what the user is offered to the relevant map() unmap() etc methods is to have a smaller interface than the whole public API offered by mediatorMap. Which is what IMediatorMap is...

> almost exactly a one-to-one
> mapping of MediatorMap's public interface.

The *almost* there is very important. It doesn't matter (IMO) whether it's different by one method or more than one method.

Surely the intent is much more important than the implementation details?

Having decided that we want to ensure that the user is never allowed to use a method that they shouldn't be using, if the way to achieve that is through an interface, what's the actual problem?

I simply cannot see where the cost is in having an interface that funnels the user into the sensible choices and stops them using the things they shouldn't be using... (once the documentation etc is complete, and the injector is restored to its more helpful behaviour). The average developer will make use of the API on the mediatorMap many, many more times than they inject it.

---

An additional reason for keeping the interfaces (putting aside modular):

Mocking an interface is fast, mocking a concrete class is slow. The testing I do on commands and configs is often based on mocking the maps and injector and verifying that things have been gathered from events and then mapped in the expected ways.

I'm actually about to do some performance testing on mocking the injector (vs an interface for it). I will possibly end up wrapping it in a delegating class, mapping that against an interface and injecting against that myself. Which - as you can imagine - makes me a very sad panda, but I don't want the hit to my test suite if it turns out to be as significant as I think it will be.

Sx

[Neil Manuell]

@Alec look what you started!

[Till Schneidereit]

On Thu, Jul 12, 2012 at 4:07 PM, Stray <dailys...@googlemail.com> wrote:
> Darn, this got long. Sorry.

This stuff is complicated (or complex, at the very least), so don't be :)

>
>> This seems to indicate
>> that there shouldn't be a publicly available MediatorMap at all
>> because you should *never* instantiate that yourself.
>
> Agreed - and we already agreed *somewhere* that we would ship RL2 with a strong health warning that classes in the impl packages are for the RL2 framework's consumption only - you should only access API / DSL packaged classes.
>
> Unfortunately if we put them in a proper 'restricted' package a la PMD then their usage inside the extensions themselves would also flag up as 'illegal' - which it isn't.
>
> This is also why it can't be packaged internally - because it has to be available for extensions (one extension might rely on other maps). We did talk about custom namespaces as well, and decided that it wasn't ideal - perhaps we need to revisit that, but I think Joel's use case shows that it's overkill.

I personally might be in favor of using a custom namespace: That's
precisely what they're for and what I'm using them for in
Swiftsuspenders. See how it took Alec quite some time to figure things
out. Sure: he might have RTFM had it existed, but whom are we kidding,
really? (Meaning: yes, some, maybe most, people won't run into this
problem because they read the docs, but many will because they just
dive in and don't read docs or forget about this part or whatever.)

>
> So the 99% rule is that unless you are an extension developer you should only import things in the API and DSL packages.
>
> Alec is a little ahead of the game, so I'm not sure we have documentation spelling that out yet, or even whether we've done our final pass through the classes to check which package each belongs in. In *most* cases those classes will have (currently) an IClaudius look to them. The exceptions (other than events and errors) being classes such as Mediator which are intended for you to extend. (Again I *like* that clarity: inject against IClaudius, but don't try to extend IClaudiuses!)
>
>> Concrete classes can implement as many interfaces as the use-case
>> requires, of course. But I (still) don't really see why MediatorMap
>> should implement IMediatorMap, which is almost exactly a one-to-one
>> mapping of MediatorMap's public interface.
>
> I think it's actually so simple that you're looking for something bigger.
>
> The mediatorMap also implements IViewHandler.
>
> The IViewHandler methods (in this case the handleView method) are not for public consumption. Their presence in the api is confusing. They are not repeated in the IMediatorMap interface (your comment about repeating in a 3rd interface confused me as we're not doing that here).
>
> The only way to restrict what the user is offered to the relevant map() unmap() etc methods is to have a smaller interface than the whole public API offered by mediatorMap. Which is what IMediatorMap is...

So isn't the MediatorMap really a
MediatorMapAndMediatorMappingViewHandler, then? It might very well be
that that is the best way to implement things by far, but then that's
exactly that: an implementation detail which should definitely be
hidden behind an interface. And this is the point at which I'd again
say that I think the MediatorMap should always what you're interacting
with and that you shouldn't care about whether it's an interface or a
class. Same with the ViewHandler, which in this case just so happen to
be implemented by the same class (and, in a default Robotlegs 2 MVCS
app, represented by the same instance, even).

The advantages of this clear-cut, role-based naming of actors in the
system seem to outweigh the downsides by far, to me, which is why I
keep on harping on them. At the same time, I'm truly convinced that
the arguments against it must be stronger than I perceive them to be:
you wouldn't hold onto them as strongly otherwise.

>
>> almost exactly a one-to-one
>> mapping of MediatorMap's public interface.
>
> The *almost* there is very important. It doesn't matter (IMO) whether it's different by one method or more than one method.
>
> Surely the intent is much more important than the implementation details?

Agreed, that's a fair point. But see above about MediatorMap then
being mis-named. (And probably conflating two separate pieces of
functionality, which might be the right choice implementation-wise.)

>
> Having decided that we want to ensure that the user is never allowed to use a method that they shouldn't be using, if the way to achieve that is through an interface, what's the actual problem?

No problem whatsoever with that.

>
> I simply cannot see where the cost is in having an interface that funnels the user into the sensible choices and stops them using the things they shouldn't be using... (once the documentation etc is complete, and the injector is restored to its more helpful behaviour). The average developer will make use of the API on the mediatorMap many, many more times than they inject it.
>
> ---
>
> An additional reason for keeping the interfaces (putting aside modular):
>
> Mocking an interface is fast, mocking a concrete class is slow. The testing I do on commands and configs is often based on mocking the maps and injector and verifying that things have been gathered from events and then mapped in the expected ways.
>
> I'm actually about to do some performance testing on mocking the injector (vs an interface for it). I will possibly end up wrapping it in a delegating class, mapping that against an interface and injecting against that myself. Which - as you can imagine - makes me a very sad panda, but I don't want the hit to my test suite if it turns out to be as significant as I think it will be.

I never heard of that problem and a quick search only turned up
performance problems with mocking classes with a long inheritance
chain and/or a very large API. Neither is true for Injector: And
interface covering it would necessarily have exactly the same amount
of public members as Injector itself. That being said, I'm looking
forward to seeing the results of your testing: facts definitely trump
idle assumptions in this (as in every other) case.

[Alec McEachran]

Sorry for turning over that particular rock everyone. I appreciate the strains that democracy can cause to projects like this, and if you've finally found compromise it is unfair of me to disturb the peace. Entertaining though!

One point from earlier in the conversation: Under the hood, there's some sort of mapping injector.map(IMediatorMap).toSingleton(MediatorMap). In what sense then is MediatorMap 'unmapped'? Isn't it explicitly mapped as a provider and might that then be marked as not-for-public-injection, or at least warning-worthy? Is it worth considering treating this differently than a genuinely unmapped class that could be automagically mapped?

I make the point with a giddy sense of anticipation! How much discussion can a trivial point provoke!? It felt like a serious point when I typed it, but it has a whiff of troll about it too...

[Stray]

I think Alec would have made his diagnosis within minutes if the naughty injector had told him there was nothing mapped to MediatorMap :/

It seems like we're back to the naming discussion rather than the 'do we need an interface' discussion? I've already said plenty on the IClaudius one - probably no value in restating it :)

> I never heard of that problem and a quick search only turned up
> performance problems with mocking classes with a long inheritance
> chain and/or a very large API. Neither is true for Injector: And
> interface covering it would necessarily have exactly the same amount
> of public members as Injector itself. That being said, I'm looking
> forward to seeing the results of your testing: facts definitely trump
> idle assumptions in this (as in every other) case.

It rather depends on the size of your test suite!

So - my quick test shows that preparing the Injector takes 150 ms, preparing an interface for it takes 50 ms. That's the whole time lapsed from the start of that process to the first test actually running - so I also created a control that doesn't actual prepare anything but just immediately kicks back the Async process, and that took 20ms.

So - in real terms it's 130ms vs 30ms for the concrete-vs-interface part.

Typically you would only do that once per test-case per mocked class. Actually creating the instances takes a wee bit longer as well, but that's insignificant - ie 100 ms to do 10000 nice() instances vs 80 ms.

But an extra 100ms (or 300% - not sure whether it's fixed or proportional) each time does add up...

If I replaced every incidence of the core robotlegs interfaces (IInjector, ICommandMap, IEventMap, IMediatorMap etc) that I mock with a concrete one I think it would add between 30 seconds and a minute to my full test run, and several seconds to the tests that I run more frequently. (Yes, it's a big test suite, but it's not like Robotlegs isn't intended for large projects).

Not sure if you consider that to be significant or not?

Personally, I'm quite focussed on squeezing performance out of my tests, and there are few bigger levers to pull on than this one.

[Stray]

Ha! Don't worry - it seems that the silent-failing wasn't really fully explored anyway - it has horrible implications for the signal command map and so on, so will probably be an optional setting rather than default, as it should definitely have screamed at you :)

That's an interesting diagnosis case for InspectorGadget though - anything mapped-to-a-something-else shouldn't be injected as a concrete type (or at least we should tell you that it's kind of a weird thing to do).

[Stray]

So - with a little bit of direction from Till, I've been playing around with solutions that are both flexible and don't fail silently, for the various thorny injection issues we've been discussing here.

What I've implemented is explained here:

https://github.com/Stray/SwiftSuspenders/blob/fallbackProviders/fallbackProviders.md

Till still needs to go over it from a technical point of view, but we can have a think about whether it's a good solution or not.

My goal was primarily to make it easy to predict the behaviour of the injector in each situation, and to make technical the barrier to setting up situations where behaviour might be unpredictable quite high (though still simple), so that it's always done with intent and not casually.

There's also this discussion on the issues on the main Swiftsuspenders repo:

https://github.com/tschneidereit/SwiftSuspenders/issues/36

Looking forward to more discussion...

Stray