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

FactoryProvider doesn't seem to respect TyphoonScope #180

Closed
jervine10 opened this issue Feb 21, 2014 · 8 comments
Closed

FactoryProvider doesn't seem to respect TyphoonScope #180

jervine10 opened this issue Feb 21, 2014 · 8 comments

Comments

@jervine10
Copy link
Contributor

I have a Factory Provider defined where I have three dependencies that I want injected and I'll provide a parameter on my own at run time. My factory looks like this:

@protocol AWWeatherStripViewControllerFactory 

@property (nonatomic, strong, readonly) AWStripViewController *stripViewController;
@property (nonatomic, strong, readonly) AWPresentationController *presentationController;
@property (nonatomic, strong, readonly) AWRefreshController *refreshController;

- (AWWeatherStripViewController *)weatherStripViewControllerWithUserLocation:(AWUserLocation *)userLocation;

@end

When the app runs, all my dependencies are provided and a WeatherStripViewController is created successfully. However, it seems like the stripViewController dependency is a singleton that gets shared across all the of the WeatherStripViewControllers that get created. This is puzzling to me because my assembly defined the definition like this:

- (id)stripViewController {
    return [TyphoonDefinition withClass:[AWStripViewController class] properties:^(TyphoonDefinition *definition) {
        definition.scope = TyphoonScopePrototype;
    }];
}

I would expect a unique StripViewController every time a WeatherStripViewController is created from this factory. This seems like a bug to me. If it is not, I can move this to StackOverflow.

@drodriguez
Copy link
Contributor

Not exactly a bug, but it might be one. The factory is working as expected: each time a factory instance is created, one instance of each property is created, and it is kept around until such factory is destroyed. Two instances of a factory will have different instances of the dependencies (unless the dependency is a singleton, of course). If you think about it, the factory doesn’t hold the definitions, but the final objects themselves.

This might not be what you expect. Right now I can only offer a work-around:

// Change the protocol as follows (basically remove the explicit dependencies)
@protocol AWWeatherStripViewControllerFactory
- (AWWeatherStripViewController *)weatherStripViewControllerWithUserLocation:(AWUserLocation *)userLocation;
@end

// In your assembly
#import "TyphoonAssistedFactoryBase.h" // not imported when doing #import "Typhoon.h"
- (id)weatherStripViewControllerFactory {
  return [TyphoonFactoryProvider withProtocol:@protocol(AWWeatherStripViewControllerFactory)
          dependencies:^(TyphoonDefinition *definition) {}
          factory:^id (TyphoonAssistedFactoryBase<AWWeatherStripViewControllerFactory> *factory, AWUserLocation *userLocation) {
            YourAssemblyName *componentFactory = factory.componentFactory;
            return [[AWWeatherStripViewController alloc]
                    initWithStripViewController:[componentFactory stripViewController]
                    presentationController:[componentFactory presentationController]
                    refreshController:[componentFactory refreshController]
                    userLocation:userLocation];
       }];
}

I think that will work as you want.

I will have a look at the behavior of the factory provider, because you are right that it should honor the definition scope. When I coded the original implementation, my dependencies were usually global objects, and I didn’t come across this kind of use pattern. I think the changes will be minimal and I hope nobody was supposing the current behaviour was the right one.

@drodriguez drodriguez self-assigned this Feb 21, 2014
@jervine10
Copy link
Contributor Author

Thanks for the update. I'll go with your implementation as a workaround.

drodriguez added a commit to drodriguez/Typhoon that referenced this issue Mar 9, 2014
Instead of keeping the values inside the factory instance, just invoke the
component factory all the time. The component factory honors every scope
Typhoon has, and will honor any future scopes.

See appsquickly#180
@drodriguez
Copy link
Contributor

Hi @jervine10, sorry for the delay. I think I fixed your problem with the scopes. I have tested it, but I will like you to test it with your specific problem.

You can use the version of Typhoon with the fix using the following in your Podfile

pod 'Typhoon', :git => 'https://github.com/typhoon-framework/Typhoon.git', :commit => '31b2974a895df0ebd8cda7dca90ffc2362084946'

As soon as you think it is ready, I will merge it with the mainline and will be available in the next Typhoon release.

@jervine10
Copy link
Contributor Author

Thanks! I'll take a look at it and let you know how it goes. Thanks for
including me on this one.

On Mar 9, 2014, at 16:48, "Daniel Rodríguez Troitiño" <
[email protected]> wrote:

Hi @jervine10 https://github.com/jervine10, sorry for the delay. I think
I fixed your problem with the scopes. I have tested it, but I will like you
to test it with your specific problem.

You can use the version of Typhoon with the fix using the following in your
Podfile

pod 'Typhoon', :git =>
'https://github.com/typhoon-framework/Typhoon.git', :commit =>
'31b2974a895df0ebd8cda7dca90ffc2362084946'

As soon as you think it is ready, I will merge it with the mainline and
will be available in the next Typhoon release.

Reply to this email directly or view it on
GitHubhttps://github.com//issues/180#issuecomment-37138571
.

@jervine10
Copy link
Contributor Author

Alright @drodriguez , this update is working as expected now. Appreciate it! 👍

drodriguez added a commit to drodriguez/Typhoon that referenced this issue Mar 16, 2014
Instead of keeping the values inside the factory instance, just invoke the
component factory all the time. The component factory honors every scope
Typhoon has, and will honor any future scopes.

See appsquickly#180
@jervine10
Copy link
Contributor Author

@drodriguez Do you guys have an plans on putting out a new release any time soon? I've integrated the commit you pointed me to above into our project and it's working, however there is one small issue. If I run pod update then try to build the app, the build fails because of missing files. TyphoonAssembly.m and TyphoonAssemblyAdviser.m are importing OCLogTemplate.h, but this file does not exist. Not a huge deal because I can revert the removal of those files, but more of a maintenance issue. Thanks!

@drodriguez
Copy link
Contributor

@jasperblues is there a “protocol” to built a new patch release? Is the checklist written somewhere? I think I still have write access to Cocoapods repositories, but I will like to know the process you follow for the releases.

@jervine10 I rebased those commits to merge them into master, so it will not longer exist and may produce problems on the long run. Right now you can track the master branch and it should be more stable than that old commit.

@jasperblues
Copy link
Member

@drodriguez @jervine10 We generally practice "release early, release often" . . so as soon as we have something of value we tag and push to CocoaPods. . however, at the moment we are pushing towards Typhoon 2.0

For the meantime, I recommend loading from CocoaPods like this

pod 'Typhoon', :head

this will allow access to new features. Typhoon 2.0 will be pushed to CocoaPods, once this release checklist is done: #193

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

3 participants