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

Non-invasive version of TyphoonComponentFactoryAware #131

Closed
jasperblues opened this issue Dec 10, 2013 · 17 comments
Closed

Non-invasive version of TyphoonComponentFactoryAware #131

jasperblues opened this issue Dec 10, 2013 · 17 comments

Comments

@jasperblues
Copy link
Member

We have a non-invasive version of TyphoonPropertyInjectionDelegate (by that I mean your class doesnt' have to have any dependencies on Typhoon). .

. . we need the same for TyphoonComponentFactoryAware . . I guess this would be just injecting the factory from the block/XML-style assembly

Implementation:

  1. Define a component called TyphoonFactoryInjector that is factory aware.
  2. Add it to your assembly. . (or its added by default? )
@jasperblues
Copy link
Member Author

Planning to implement this in the next 48 hours. . .

@jasperblues
Copy link
Member Author

Here’s how I plan to do it:

  • We can do type matching on TyphoonComponentFactory and implement the injector in TyphoonComponentFactory+InstanceBuilder
  • Since TyphoonAssembly (and sub-classes) only relate to the block-style assembly I don’t want to couple the base assembly to these classes. . . instead I will make the block-style assembly register a TyphoonComponentFactoryPostProcessor for itself. . and this post processor will like for any injects by type requiring a TyphoonAssembly or sub-class and instead inject the factory (since the assembly will be posing in front of the factory - that’s how it works). . . this way we keep the separation of concerns.

@alexgarbarev
Copy link
Contributor

Jasper, did you start working on this issue or it's still open for anyone?

@jasperblues
Copy link
Member Author

I got caught up in some other things (TCF+InstanceBuilder - refactor to create NSInvocation without requiring initTarg instance). . . you can take it.

@jasperblues
Copy link
Member Author

Once this feature is done, we should update this StackOverflow question: http://stackoverflow.com/questions/20405636/typhoon-injecting-a-view-controller-provider/20407286#20407286

  • Option 1: TyphoonComponentFactoryAware
  • Option 2: Inject assembly interface (this feature)
  • Option 3: TyphoonFactoryProvider

@jasperblues
Copy link
Member Author

If you take #153 can I take this back again tomorrow?

@alexgarbarev
Copy link
Contributor

#153 is implemented. You can take this issue

@alexgarbarev
Copy link
Contributor

Now this issue implemented and waits for review

@alexgarbarev
Copy link
Contributor

I did it a bit different, not so much as you've suggested above. Please review and refactor if needed.

@jasperblues
Copy link
Member Author

It works fine, however I think its important to keep the TyphoonBlockComponentFactory concerns separate from the base, which is why I suggested using a TyphoonComponentFactoryPostProcessor.

To do this we'd need:

  • [definition injectProperty:@selector(someProperty) withFactory]; <---- We need to introduce this style anyway for initializers

. . the post processor would then run over the block assembly and convert any auto-injected assemblies to inject the factory.

@alexgarbarev
Copy link
Contributor

[definition injectProperty:@selector(someProperty) withFactory];

Can't be done in objective-c. You cannot use withFactory without argument after keyword.

I agree with using TyphoonComponentFactoryPostProcessor for assemblies. I'll try to do it.

Also about method naming: I want to note that using register as method name is not good idea because it is reserved keyword (as const or id), so I cannot cmd+click to this method and also it hightlighted with wrong color (in xcode at least). Better is to use registerDefinition: and registerPostProcessor:

@jasperblues
Copy link
Member Author

Oops. . . how did I write that!?!?

I meant:

[definition injectWithFactory:@selector(someProperty)];

Yes, lets rename register. . . AppCode has been showing me that its a reserved keyword for a long time, but nothing has been done about it yet. . . Its basically an internal method (unless someone wants to use the base factory directly) so we can rename it with minimal impact.

@alexgarbarev
Copy link
Contributor

Ok.
I used method name

[definition injectPropertyWithComponentFactory:@selector(property)]

because we already used factory word in definitions methods to specify some objects factories (not TyphoonComponentsFactory) - see method:

- (void)injectProperty:(SEL)selector withDefinition:(TyphoonDefinition *)factoryDefinition selector:(SEL)factorySelector;

What do you think about method naming? How should we indicate to user that we will inject TyphoonComponentsFactory - not SomeObjectFactory

@jasperblues
Copy link
Member Author

👍

alexgarbarev added a commit that referenced this issue Feb 16, 2014
Now factory/assembly property injection-by-type replaced by postprocessors with injection-with-component-factory
@jasperblues
Copy link
Member Author

Pushing this as 1.8.0. . although we may refactor internally, useful as-is.

@alexgarbarev
Copy link
Contributor

What would you like to refactor for this issue?

@jasperblues
Copy link
Member Author

Oops.

We discussed using a PostProcessor instead of making the base TyphoonComponentFactory aware of the block style.

It looks like you've already implemented that.

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

No branches or pull requests

2 participants