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

TyphoonAssistedFactoryProvider : Don't consume dependencies until factory is used. #145

Closed
jasperblues opened this issue Jan 16, 2014 · 10 comments
Labels

Comments

@jasperblues
Copy link
Member

No description provided.

@drodriguez
Copy link
Contributor

Can you elaborate a little bit more? Do you mean keeping the properties as “lazy”?

@jasperblues
Copy link
Member Author

Yes, I mean that if we use a TyphoonAssisted factory, to provide an option (the default?) to not load the property dependencies until the time that we invoke a factory method.

In this way we can have:

  • ViewControllerA
  • ViewControllerB

ViewControllerA uses a factory to transition to view controller B. And B has a complex (memory hogging) object graph. That way we're only using one object graph at a time.

The simple way to do this would be:

  • Create the implementation of the property so that it looks up the component from the container. From the user's point of view this still looks and works exactly like DI. . . ie - if the application assembly was changed, everything would follow-suit.

    (in fact it's how Spring's @configurable annoation works to provide dependency injection on non-container managed objects).

Eg:

_Custom run-time override of a property_

- (MyPropertyType*)myProperty
{  
    //I suppose this will need to be @synchronized too
    if (_myProperty == nil) 
    {
         //Load my property from the container
    }
    return _myProperty;    
}

@drodriguez
Copy link
Contributor

Ok. Let me see if I can explain how I face this problems (and why what you propose might not be possible):

First: if one of the dependencies of a factory is quite big, building it before hand, when the factory is instantiated looks like a bad thing to do. I take it like a code smell, and go back to the drawing board. Maybe there is another way to do the same thing? Maybe I should have the factory for ViewControllerB instead? Normally the dependencies should be singleton scope instances, that should be already built when the factory is first instantiated, so there is no cost.

Second: if you decide to “have a factory for ViewControllerB” you might start thinking that this is factories all the way down, and you will probably be right. One thing that you can do is cheat a little and use a block that gives you a ViewControllerB instead of a ViewControllerB directly. You will need to use the factories: or factory: methods, because the automagic method will not work for that case.

- (id)viewControllerC
{
  return [TyphoonFactoryProvider withProtocol:@protocol(ViewControllerCFactory) dependencies:^(TyphoonDefinition *definition)
  {
    [definition injectProperty:@selector(viewControllerB) withObjectInstance:^id {
      return [[ViewControllerB alloc] init];
    }];
  } factory:^id (id<ViewControllerCFactory> factory, id somethingElse) {
    return [[ViewControllerC alloc] initWithViewControllerB:factory.viewControllerB() somethingElse:somethingElse];
  }];
}

As you can see, not that hard, but a little more verbose. The problem is that ViewControllerB has to be built “manually”, which is not optimal, since any definition of ViewControllerB in the assembly will not be used. You can substitute the block to use this other return:

return [[TyphoonComponentFactory defaultFactory] viewControllerB];

This will use your definition, but only if viewControllerB is actually in the default factory.

As far as I know there is no way to get the factory associated to the assembly at runtime (except that defaultFactory trick), but maybe it should be one. Having this will allow also using the definition of ViewControllerC as the “body” for the factory method (maybe).

These problem is why originally the body is a block and not just a definition, and it might be the reason for #144 (since the object built never “passes” through Typhoon).

@jasperblues
Copy link
Member Author

I think with mobile the dependencies don't need to be singletons. .. just an object-graph for the current use-case(s).

I use this kind of thing all of the time - just load one object graph (view controller and deps) and throw it away after transitioning to the next. . .

Therefore, I think its reasonable that ViewControllerA should only load it's own dependencies and not those by some view controller it will transition to. . . especially if that could be one of 10 depending on what happens in A.

The factory reference should be quite simple:

What if we add a property to NSObject (via an associative reference) to point to the factory that created it? . . it could be a weak reference too, so no cost (except the small runtime hit of using weak refs). . . . though if we're using lazy, it would have to be a strong ref.

 (id)factory {
    return objc_getAssociatedObject(self, &factoryKey);
}

- (void)setFactory:(id)factory {
    objc_setAssociatedObject(self, &factoryKey, newObjectTag, OBJC_ASSOCIATION_RETAIN_NONATOMIC);
}

@jasperblues
Copy link
Member Author

relates to: #131

@drodriguez
Copy link
Contributor

Sorry, I didn’t express myself correctly: I meant that TyphoonAssembly might have some kind of property to the factory that it has “created” during runtime, not any arbitrary object.

About the singleton or not, you are right, what I meant to say was that dependencies (in our case) were normally singletons (storages and such). I don’t think that we ever needed to inject another view controller or such a thing. Normally view controllers were actually injected as factories (which again should be cheap), and the dependent object will create one view controller (or many) when it needs it. This forces the factory to create the dependencies, which depends on the use case can be very bad. The block trick and using the defaultFactory usually works to avoid that pre-loading (if you don’t need every instance having the same dependencies).

I can have a look to see how hard will be gettting that referece to the factory injected to the assembly (I have to read carefully the collaborating assemblies, because those look like edge cases), and maybe implement this lazy behaviour you talk about (I don’t think that defaulting to it will be possible, because that part of the code is plain Typhoon). I don’t know when I could look into it, though.

@jasperblues
Copy link
Member Author

Ah ok! Sorry for misunderstanding.

That should be very simple then. It is in fact #131, which is required for a few other reasons. . I think I can easily do that - let me do that first. Then I might come back and ask you how to proceed with the TyphoonAssistedFactory mods.

_A Note about collaborating assemblies_

They're still the same factory underneath - just like if you'd split an XML assembly up into multiple docs. As yet Typhoon still has no explicit support for parent / child factories.

  • The advantage of the block-style approach is you can put just the objects being consumed on the public interface, with any child components used to support the main object being private. . . they're still all defined in the same container though. . .

@ghost ghost assigned drodriguez Jan 30, 2014
drodriguez added a commit to drodriguez/Typhoon that referenced this issue Jan 30, 2014
TyphoonPropertyInjectionInternalDelegate exposes a new hook for property
injections, in which the target instance will be asked if the injection
should proceed or not.

Instances can implement the hook and perform the injection themselves (using
the value provided by the InstanceBuilder or whatever they please).

Relates appsquickly#145
drodriguez added a commit to drodriguez/Typhoon that referenced this issue Jan 30, 2014
TyphoonAssistedFactoryBase uses the new shouldInjectProperty:withType:lazyValue:
to store the lazy value block in its internal map, and telling Typhoon to not
inject the property.

The creation process no longer creates the setter (not needed anymore), but
creates an ivar for each property (same name as the property).

Later, when the getter of the property is invoked, if the ivar is still nil,
the lazy value block is invoked, saved into the ivar and returned to the
caller. The properties are synchronized to avoid two threads creating the ivar
at the same time.

See appsquickly#145
@drodriguez
Copy link
Contributor

Seems that it was a little bit easier than what we talk:

  • I created another hook point when the properties of an instance are set by Typhoon.
  • I “package” the call to valueToInjectProperty:withType:onInstance: in a block. This block is sent as a parameter to the hook.
  • TyphoonAssistedFactoryBase implements the hook and stores the block without invoking it.
  • When the getter is called, the block is invoked, extracting the value, and storing it into an ivar.

I think it covers your desidered use case. There is no opt-out (implementing both systems will be very annoying), but I cannot see how being lazy by default could harm anyone.

Have a look (it’s in a branch), and tell me 👍 or 👎 . I will rebase and merge into master.

@jasperblues
Copy link
Member Author

👍

Great!

@jasperblues
Copy link
Member Author

Looking forward to trying this!

drodriguez added a commit to drodriguez/Typhoon that referenced this issue Jan 31, 2014
TyphoonPropertyInjectionInternalDelegate exposes a new hook for property
injections, in which the target instance will be asked if the injection
should proceed or not.

Instances can implement the hook and perform the injection themselves (using
the value provided by the InstanceBuilder or whatever they please).

Relates appsquickly#145
drodriguez added a commit to drodriguez/Typhoon that referenced this issue Jan 31, 2014
TyphoonAssistedFactoryBase uses the new shouldInjectProperty:withType:lazyValue:
to store the lazy value block in its internal map, and telling Typhoon to not
inject the property.

The creation process no longer creates the setter (not needed anymore), but
creates an ivar for each property (same name as the property).

Later, when the getter of the property is invoked, if the ivar is still nil,
the lazy value block is invoked, saved into the ivar and returned to the
caller. The properties are synchronized to avoid two threads creating the ivar
at the same time.

See appsquickly#145
drodriguez added a commit to drodriguez/Typhoon that referenced this issue Jan 31, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants