Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

instanciate non-mapped classes #36

Open
hvilela opened this Issue Oct 11, 2010 · 34 comments

Comments

Projects
None yet
5 participants

hvilela commented Oct 11, 2010

I would like to see an flag on the injector to force it to instanciate non-mapped classes to avoid long lists of simple classes mapping. So, we could do:

injector.instanciateNonMappedClasses = true;

insted of:

injector.mapClass(ClassA, ClassA);
injector.mapClass(ClassB, ClassB);
injector.mapClass(ClassC, ClassC);
...

Owner

tschneidereit commented Oct 8, 2011

This will be supported in a slightly different fashion in Swiftsuspenders 2. Instead of just instantiating unmapped classes, it will be possible to set a default dependency provider that will be invoked after no specific provider has been found for a dependency. This provider can be used to simply create an instance of the requested class.

Contributor

darscan commented May 10, 2012

How is this looking?

Owner

tschneidereit commented May 10, 2012

Right, the default provider. I remember having some problem with it, but I'll look into that once again.

Contributor

darscan commented May 10, 2012

Am I correct that at the moment one can do this:

injector.getInstance(NonMappedClass);

and a new instance will simply be created. Whereas, if a container-constructed class has a dependency on such a non-mapped class an error will be thrown?

Symmetry!

Owner

tschneidereit commented May 10, 2012

Gah!

Yes, you are correct. And yes: Symmetry!

Maybe this even ties in nicely with my strongly held belief that interfaces should only ever be created if there are more than one implementation of something. Changing the injector to just inject new instances of a requested class if they're not mapped would mean that a mapping only ever needs to be created if an interface is requested, too - which seems nice.

On the other hand, I remember you wanting singleton mappings to be the default (still not gonna happen!), indicating that for a large number of use cases, the mapping would still be required.

Owner

tschneidereit commented Jun 2, 2012

@darscan recently made a pretty good point about there being a nice symmetry between Injector#getInstance() returning new instances for unmapped types and the injector just instantiating unmapped classes when requested dependencies. Hence, this is how it works, now.

Contributor

Stray commented Jul 12, 2012

Sorry guys, I missed this.

"I forgot to map this" is a mistake I make frequently. In RL1 that's cool, the injector has my back. But Silent Fails are a nightmare to debug.

I suspect the symmetry isn't real - the process you're entering into is different. Actually, I think it should be the other way around. These processes should be less symmetrical. If anything I think how it was before was a feature, not a bug. "Make this according to the rules I have given you" is different from "Make this".

Initially I thought that as I like interfaces I wouldn't see these silent fails anyway, but actually this is a big issue around the signal command map - if the command expects a value to be injected and it isn't passed in the signal then that's unlikely to be something that you don't want to be told about.

I know folk have complained that they can't pass null in a Signal before, but I think that's a feature not a problem, and we could have a generic NullInjectableObject that the injector doesn't choke on for those situations where folk want to do it deliberately and can't be bothered to implement their own NullObjects [conditionals be damned but hey ho].

So, I'd prefer the injector to protect me when I've forgotten to map than be all-forgiving of missing mappings for concrete classes.

If I just want to use the injector as a factory, newing up stuff as it needs to to get the job done, and not holding on to those values, there could be an explicit method for that...

injector.newThisShitUp(UnmappedClassWithUnmappedDependencies);

However - the original poster's suggestion, that this could be a flag on the injector, sounds workable to me as well (without considering how Till implements it). At least it lets me opt out of the silent fails, and we could specify that this mode is not compatible with use of signal/message-passing-object based commands.

It's also not advisable to have this flexibility when using with guards/hooks on the mediator map, where the concrete view injection might just quietly new up a view in your hook rather than help you diagnose a problem. I'm thinking of when people inject a view into a guard or hook against a (super or sub) type that they didn't specify in the TypeMatcher. I was relying on the injector to help them diagnose this problem.

On the interfaces issue: it's absolutely not that I don't believe the same as you Till - I believe you're exactly correct about the preferred situation in theory, but that without duck-typed interfaces it's not deliverable in AS3, unless we give up the nice DSLs and are prepared to make modular compilation much more cumbersome. Personally, I think the DSL alone is worth the hit of having to have interfaces, even without considering how much more difficult modular development becomes without them.

Owner

tschneidereit commented Jul 12, 2012

So I'm mostly convinced - thanks for the clear arguments against auto-instantiation, @Stray.

I'll leave this open and wait for other comments for a bit, but if nothing new comes up, I'll probably implement the flag-based solution. Both because it's easiest and because it seems closest to satisfying demand while preventing problems.

@Stray, so as not to smear that discussion all over the place, could you add yet another explanation of why class-interface dualism is required for the base case in DSLs to the mailing list discussion? Sorry for being so dense, but I honestly still don't get that point. (I get, but don't agree with, the point about increased difficulty of modular development.)

@tschneidereit tschneidereit reopened this Jul 12, 2012

Regarding this question I would suggest to follow The zen of python:

Explicit is better than implicit.
Errors should never pass silently.
There should be one-- and preferably only one --obvious way to do it

(witch I did in my mvc implementation and it works like a charm.)

just my two cents...

Owner

tschneidereit commented Jul 12, 2012

There should be one-- and preferably only one --obvious way to do it

What's the one obvious way to do it? And what exactly is it in this case?

proxyMap.map(new MyProxy(), IMyOptionalProxyInterface, "optionalName");

If I need a proxy... I just map it. If I need it only sometimes... I have commands to handle that.
I hate any automation regarding proxy classes, as they form a base of application.

Owner

tschneidereit commented Jul 12, 2012

I'm not sure I can follow. What are "proxy classes" in your framework
and how do they relate to automatically instantiating non-mapped
concrete classes when they're requested for injection in
Swiftsuspenders?

Hm... Good point.

In both cases injection is used... but we use different approaches.

Your approach is universal and generic, mine is tied to framework and does what I need and only what I need.(to make it run fast and avoid bloated implementation..)

Ignore me.

Contributor

Stray commented Jul 13, 2012

Can you clarify why you decide to do away with instantiate and only have getInstance? Thx.

I feel like constructor injection is orthogonal to mapping... and yet the current situation is that I can make a new instance and injectInto it, which only locks me out of constructor injection. That feels... spurious, but perhaps there's a damn good reason?

Contributor

Stray commented Jul 14, 2012

Extra thought: the current situation also locks me out of [PostConstruct].

(Unless InjectInto also runs PostConstruct methods? But that seems confusing...)

At first I thought just an instantiate() method would do away with the problem the OP describes, but of course he's interested in trees, not just top level. I'm guessing that noodle is why instantiate isn't currently in there...

Owner

tschneidereit commented Jul 14, 2012

I'm not sure I understand what you're getting at. getInstance works like this:

  • if the given type is mapped, return whatever the mapped provider returns
  • otherwise, use ctor injection to create a new instance of the given
    type (if it is a class, not an interface, of course, throw otherwise)

That is, it does exactly what instantiate used to do for all unmapped types.

Contributor

Stray commented Jul 14, 2012

Agreed... but I think we've already identified that it shouldn't?

Because we need the errors (unless the user deliberately switches them off).

Owner

tschneidereit commented Jul 14, 2012

Ah, now I understand what you're getting at. This is a different topic
altogether, though, as the removal of instantiate is not related to
the instantiation of unmapped dependencies at all.

For some discussion that lead up to the removal of instantiate, see
issues #15 and #25.

Basically, the thinking is that getInstance should always return an
instance; the value returned by a dependency provider if that exists,
or a newly (through ctor injection) created instance for unmapped
classes.

As getInstance is only ever called by explicit decision of the API
user, I don't think that's a similar situation to automatic, behind
the scenes instantiation of unmapped dependencies.

Also, the amount of confusion about getInstance vs. instantiate
clearly shows that those two where to similar to each other.

Contributor

Stray commented Jul 14, 2012

"As getInstance is only ever called by explicit decision of the API
user, I don't think that's a similar situation to automatic, behind
the scenes instantiation of unmapped dependencies."

Except in the extensions that rely on it... though if we do proper testing on those we won't be affected.

I hadn't fully appreciated the getInstance manual / automatic differences.

Still... that feels like possibly more of a case for instantiate and instantiateTree (or something - where you want to new stuff up all the way down). (And yes, too similar in naming).

The thing is, it seems to me to be perfectly valid (particularly within an extension setting) that you might want a new instance, regardless of any that is mapped as a Singleton. Are those two things necessarily related?

But... now I shall go and read those issues 15 and 25 and get myself up to speed on what I've missed (as I'm sure there are good reasons).

Contributor

Stray commented Jul 14, 2012

Ah... I may be being stupid, could one simply get a childInjector, make a new mapping and getInstance on that childInjector if you wanted a fresh instance?

Owner

tschneidereit commented Jul 14, 2012

Ah... I may be being stupid, could one simply get a childInjector, make a new mapping and getInstance on that childInjector if you wanted a fresh instance?

That is certainly possible, yes. Or you could use
instantiateUnmapped from the internal swiftsuspenders namespace.
This is the one thing that still irks me: For creating custom
dependency providers, you will most likely want to use
instantiateUnmapped so as not to let the provider recursively invoke
itself. Maybe I should just go and make instantiateUnmapped public
again?

Contributor

Stray commented Jul 14, 2012

Yes please!

I actually think there are 4 distinct behaviours here and we're trying to cram them into one or two API functions.

  1. Get me an instance, if there is a valid mapping, and error if not.
  2. Get me an instance, using the mapping if it exists, and just creating a new one if it doesn't.
  3. Get me a brand new instance, regardless of mappings, but using mappings for the dependency tree.
  4. Get me a brand new instance, with a brand new dependency tree.

For me, those fall on to:

  1. getInstance( type : Class, name : String = '', targetType : Class = null );
  2. getOrCreateInstance( type : Class, name : String = '', targetType : Class = null );
  3. createNewInstance( type : Class, useMappingsForDependencies : Boolean = true ); // passing true
  4. createNewInstance( type : Class, useMappingsForDependencies : Boolean = true ); // passing false

For 3 and 4, the other option, already supported, is to use a custom default dependency provider, but that then requires you (in the case of having both 3 and 4) to track which of those is which in a second location in your code, which is a bit icky.

I think this would cover all the concerns folk had across issues 15 and 25 (though I expect my reading suffered a little from bias ;) ).

Contributor

Stray commented Jul 14, 2012

For clarity - I've gone for createNewInstance because instantiateUnmapped implies that the class being instantiated mustn't be mapped, where as createNewInstance doesn't. There may be a better fit.

Owner

tschneidereit commented Jul 14, 2012

Have to think about the others some more, but just as a quick
question: What on earth would 4 be useful for? And what would it do
for interface dependencies?

Contributor

Stray commented Jul 14, 2012

4 would be the solution to the OP's problem - where he has a nested set of dependencies (think army of Robots each needs 2 legs, 2 arms, legs need knees, arms need elbows etc) and doesn't want to have to map(RobotToe).toType(RobotToe) for each body part.

At this point you're just using the injector as a smart factory. Whether that's a valid use case I'm not sure, but it seemed to be something people wanted.

If you do 4 with interface dependencies then Computer Says No. (Noisy but helpful error).

Owner

tschneidereit commented Jul 14, 2012

Thanks for the explanation/ reminder. Will have to mull all of this over some.

Contributor

Stray commented Jul 14, 2012

Yes. History shows the discussion has rumbled on for 3 years - I'm guessing an afternoon isn't going to be sufficient :)

Contributor

darscan commented Jul 14, 2012

IMHO I think most use cases could be covered (whilst keeping things sane, and hopefully easy to implement) by two things:

1 - A flag on the injector to toggle how unmapped classes are treated.

Naming is difficult here. I've been mulling over strict (true by default). Or... is this simply a matter of not having a default provider? Could the flag simply add/remove the default provider?

2 - A method that allows one to simply new up instances (as per the old instantiate). I'm thinking createInstance() might be a better name?

I was unhappy about the removal of instantiate originally, but the only places I've needed it is inside extensions and for those we always use child injectors so getInstance works perfectly fine (provided that the toggle exists to allow instantiation of unmapped classes).

I think what I'm getting at is that createInstance/instantiate might be unnecessary if we can turn off strict for our child injectors, whilst leaving strict on for the context's injector (for developer safety and feedback).

Contributor

darscan commented Jul 14, 2012

I think what I'm getting at is that createInstance/instantiate might be unnecessary if we can turn off strict for our child injectors, whilst leaving strict on for the context's injector (for developer safety and feedback).

Damnit, that doesn't quite cover the needs of the SignalCommandMap though, were we need the ability to instantiate the Command class (unmapped) but explode if the command specifies any unmapped dependencies...

Contributor

Stray commented Jul 14, 2012

Yeah, that was my original "Well, this ain't gonna work!" example.

I think the problem with the flag is similar - that moment-to-moment I may vary what I want the injector to do with an unmapped dependency. It seems saner (to me) to be explicitly stating that as you make the demand of the injector, not having it hidden elsewhere and set global-ish.

I say global-ish because we haven't talked about the child/parent injector's response to setting the flag on one specific injector. If it doesn't trickle up, and I want to set it globally, do I need to do injector.rootInjector.... ? Hrm.

Nightmare scenario: someone else decides to set the flag on the injector somewhere that may even be in a different module (if it can be set / unset at any time) and depending on order of execution, injections sometimes error and other times are fulfilled (and even then, sometimes fail silently, because you've also forgotten to make the mapping anyway).

I guess the same is true of unmapping at runtime, but I think that would be easier to diagnose - in that one could more easily search for unmap(Thing) in the codebase and figure out how to unknot that problem.

If the flag is locked at startup, that seems insufficiently flexible.

On 14 Jul 2012, at 17:37, Shaun Smith wrote:

I think what I'm getting at is that createInstance/instantiate might be unnecessary if we can turn off strict for our child injectors, whilst leaving strict on for the context's injector (for developer safety and feedback).

Damnit, that doesn't quite cover the needs of the SignalCommandMap though, were we need the ability to instantiate the Command class (unmapped) but explode if the command specifies any unmapped dependencies...


Reply to this email directly or view it on GitHub:
#36 (comment)

Contributor

darscan commented Jul 14, 2012

It seems saner (to me) to be explicitly stating that as you make the demand of the injector

That's a slightly separate issue to the original issue in this thread (although all this stuff is related, so it's tricky).

Some people, myself included, would like reduce the boilerplate of simple class mappings, and instead let the injector do the work. But that's at the "application" dependency mapping level - I still want the extensions etc to explode when things are incorrectly configured. I consider the context injector mine, but all extensions should use their own (child) injectors, where unmapped dependencies should explode.

@tschneidereit I know you are sold on the fallback dependency provider pumping out new instances willy-nilly for every request (you insane man you), but is that the sort of thing I could change myself by providing a reasonable (singleton) dependency provider? p.s. interleave lots of ;-) 's into that question!

Contributor

darscan commented Jul 14, 2012

@Stray Sorry, I didn't explain why it's a separate issue. Essentially it's to do with this part:

explicitly stating that as you make the demand

This relates to dependency resolution within the application, which happens outside of your control a lot of the time, so there is no place where you "make the demand".

If that makes sense at all.. sorry, I don't think I'm being very clear.

Contributor

Stray commented Jul 14, 2012

Ah - yes. I see - I think that cleared it up for me.

So actually there's the 4 specific scenarios I outlined when 'pulling' from the injector, plus the desire to toggle the default 'push' behaviour.

I'm inclined towards:

  1. Always blow up by default for missing mappings requested via 'push'.
  2. Provide specific explicit methods for varying the desired response where something is being 'pulled' manually from the injector
  3. Allow the user to set the default provider on an injector (and to tell the injector to inherit that from its parent, or not). This represents a slightly higher barrier technically when compared with a flag/switch, but IMO that's a good thing ;) "I'll just switch this flag" feels different from "I'll just change the default dependency provider" in terms of the casualness with which someone might make that change.

On 14 Jul 2012, at 18:14, Shaun Smith wrote:

It seems saner (to me) to be explicitly stating that as you make the demand of the injector

That's a slightly separate issue to the original issue in this thread (although all this stuff is related, so it's tricky).

Some people, myself included, would like reduce the boilerplate of simple class mappings, and instead let the injector do the work. But that's at the "application" dependency mapping level - I still want the extensions etc to explode when things are incorrectly configured. I consider the context injector mine, but all extensions should use their own (child) injectors, where unmapped dependencies should explode.

@tschneidereit I know you are sold on the fallback dependency provider pumping out new instances willy-nilly for every request (you insane man you), but is that the sort of thing I could change myself by providing a reasonable (singleton) dependency provider? p.s. interleave lots of ;-) 's into that question!


Reply to this email directly or view it on GitHub:
#36 (comment)

Contributor

Stray commented Jul 17, 2012

So - I done forked it and implemented this, as a starting point. Deets here:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment