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

Added 'injectProperties' to allow injection of externally created objects. #15

Merged
merged 2 commits into from
May 8, 2013

Conversation

BrynCooke
Copy link

It would be really good to be able to use Typhoon with ViewControllers and Storyboards. Unfortunately UIStoryboard creates view controllers and this can't be overridden. The next best thing is to inject the properties of the view controller.

So adding an injectProperties now you can do something like this:

[_componentFactory register:[TyphoonDefinition withClass:[Knight class] properties:^(TyphoonDefinition* definition)
    {
        [definition injectProperty:@selector(quest)];
    }]];
[_componentFactory register:[TyphoonDefinition withClass:[CampaignQuest class] key:@"quest"]];

Knight* knight = [[Knight alloc] init];
[_componentFactory injectProperties:knight];

assertThat(knight.quest, notNilValue());

This means an extension of UIStoryboard can inject the view controller just after it is created.

jasperblues added a commit that referenced this pull request May 8, 2013
Added 'injectProperties' to allow injection of externally created objects.
@jasperblues jasperblues merged commit f07c821 into appsquickly:master May 8, 2013
@jasperblues
Copy link
Member

Nice work Bryn - big thanks for that!

Questions:

  1. Do you think we should provide the decorated storyboard - TyphoonStoryBoard, that performs the subsequent injenction?
  2. Would you like to update the docs? I can assign access for this.
  3. Would you like to appear on the website as a contributor? (Would require a pic and 2-3 sentence bio).

@BrynCooke
Copy link
Author

Hi,
Yes I think it would be useful to include TyphoonStoryBoard. I am happy to develop this, but would like to see how it pans out in my current project before submitting a pull request (shouldn't take very long).

Yes I can update the docs.

It would be great to appear on the website. I'll send you the details via linked in.

@jasperblues
Copy link
Member

Hi Bryn,

I've assigned you push access. The rules for push access are simply these:

No Questions Asked:

  • Allows you to freely edit documentation
  • Allows you to perform bug-fixes.

Requires Review:

  • Any experimental new features should be done on a branch, and we agree before committing to the main branch.
  • I'm pretty sure we'll agree on what features are required, but in the event unlikely that we don't, we'll consult a wider group of users.

Please keep the test coverage above 90%. . . . (Actually its currently at 88.8% because I recently added Spring-like collections, and didn't include enough tests).

Thanks for your great work :)

@jasperblues
Copy link
Member

Regarding the TyphoonStoryBoard

  • I was thinking about this a few weeks back. . . on the one hand its easier (doing the injection for you) and would allow other container services to be called for you at the same time. .. (AOP, etc)
  • On the other hand it does create a direct dependency on Typhoon in your class. . . . I think its nice when the framework is non-invasive. . . having said that I often use the ApplicationAssembly in the same way as story boards to load view controllers. .

@BrynCooke
Copy link
Author

Hi,
Thanks for this :)

I think as long as it is possible to inject the TyphoonStoryBoard as a UIStoryBoard then the separation between the app code and Typhoon is maintained.

Having a definition factory method that returns a definition for the a particular storyboard would be nice. However we will need to be able to inject the component factory (not sure if this can the done at the moment)

I have a general question before I write up the docs on 'injectProperties' as I want to get the functionality right.

Would you say that a TyphoonDefinition applies only if the type is an exact match rather than a type and all its subtypes? So if a Knight was defined with a quest then CavelryMan would not use the quest definition implicitly?

Cheers,
Bryn

@jasperblues
Copy link
Member

Can you give me an example of what you mean using code? And/or skype me on 'jasperblues'

@BrynCooke
Copy link
Author

Hi,
It's probably best illustrated with the test I added.

    [_componentFactory register:[TyphoonDefinition withClass:[Knight class] properties:^(TyphoonDefinition* definition)
                                 {
                                     [definition injectProperty:@selector(quest)];
                                 }]];
    [_componentFactory register:[TyphoonDefinition withClass:[CavalryMan class] properties:^(TyphoonDefinition* definition)
                                 {
                                     //(1) No quest has been defined explicitly for CavalryMan
                                     [definition injectProperty:@selector(hitRatio) withValueAsText:@"3.0"];
                                 }]];
    [_componentFactory register:[TyphoonDefinition withClass:[CampaignQuest class] key:@"quest"]];

    CavalryMan* cavelryMan = [[CavalryMan alloc] init];
    [_componentFactory injectProperties:cavelryMan];

    assertThat(cavalryMan.quest, notNilValue()); //(2). This quest was picked up from knight.
    assertThatFloat(cavalryMan.hitRatio, equalToFloat(3.0f)); 

    Knight* knight = [[Knight alloc] init];
    [_componentFactory injectProperties:knight];

    assertThat(knight.quest, notNilValue());

Having looked further in to this I think that the behaviour at (2) where the quest is picked up from the knight is incorrect. It should be nil to match the behaviour of the following code:

CavalryMan* calavryMan = [_componentFactory componentForType:[CavalryMan class]];
assertThat(calavryMan.quest, nilValue());

I'll fix this today.

@jasperblues
Copy link
Member

Hi Bryn

You're right - a definition contains the whole recipe in its own right. So even if there's a super-class that also happens to be registered they're unrelated. . . . although this gives a little more typing, it wins out for simplicity and flexibility.

For simple cases there's always the auto-wiring Macros like these:

@implementation AutoWiringKnight
autoWire(@selector(quest))

@EnD

the above will register and inject a component of type AutoWiringKnight. . . but just to be slightly confusing, the auto-wiring does actually pick-up the stuff from a super-class! . . This is because you're defining a definition at the class level, instead of a recipe in its own right. . .

. . personally I don't use the auto-wiring much.

@jamesdterry
Copy link

Hi - I ended up needing TyphoonStoryboard and didn't see the code ever added to the project. Is someone working on this or should I go ahead and submit a pull request? (Or did I miss it somewhere?)

Also, just as a sanity check, we use XML in my project and I ended up using XML something like this:

    <component key="someViewController" class="SomeViewController" factory-component="typhoonStoryboard">
        <initializer selector="instantiateViewControllerWithIdentifier:">
            <argument parameterName="identifier" value="SomeStoryboardID" required-class="NSString"/>
        </initializer>
    </component>

Does anyone see any issues with this technique?

Thanks much

@jasperblues
Copy link
Member

Hi James,

You're right - ability to inject "post-inject" a definition was done (meaning give Typhoon some instance and tell it to do DI based on a definition), but the 'TyphoonStoryBoard' factory component was still pending.

No issue at all with the way you've solved it - and it suits the XML style well. If you'd like, please go ahead and send a pull request.

:)

@jamesdterry
Copy link

Just FYI for anyone waiting for this, waiting for my client to give me permission to contribute the code back. Big company so the wheels turn slowly.

@jasperblues
Copy link
Member

Thanks @jamesdterry - your contribution will be very welcome :)

No pressure though - if you're not able to get approval we'll go ahead an implement the feature ASAP.

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

Successfully merging this pull request may close these issues.

3 participants