Skip to content
This repository has been archived by the owner on Jan 13, 2022. It is now read-only.

Best way to inject dependencies into Fragment provided by parent Fragment / Activity #4

Closed
ubuntudroid opened this issue Feb 1, 2019 · 19 comments
Assignees
Labels
good first issue Good for newcomers question Further information is requested

Comments

@ubuntudroid
Copy link

In Android DI you quite often share dependencies between Fragments via the parent Fragment or the Activity.

Katana allows to define dependsOn components. One would think, that you would just use that:

class MyFragment: Fragment(), KatanaTrait {
  override val component = createComponent {
    modules = listOf(
      createSupportFragmentModule(this),
      myFragmentModule,
      // some other modules
    ),
    dependsOn = listOf(MyApp.applicationComponent, (requireActivity() as KatanaTrait).component)
  }

  val dependency: MyDependencySuppliedByActivity by inject()
}

But there are multiple problems with this:

  1. Getting the Activity component the way I outlined above doesn't work as the Activity isn't available when the Fragment component is created. So we would have to by lazy {} create the component or create it in onAttach/onCreate. That in turn means that we would need to either by lazy { injectNow() } all injections in the Fragment or manually set them in onAttach/onCreate as well.
  2. As Katana does not allow overrides such chains of components obviously can lead to OverrideExceptions, especially when in the case of Activity -> ParentFragment -> ChildFragment. A good example for that is Glide which should always be bound to the hosting Activity/Fragment to ensure that it stops image requests and cleans up when the context of the ImageView is gone. So it is not unusual to find one bind<Glide> per Fragment module which becomes awkward when having child Fragments as you have to name all those binds. Similar things can happen with nested NavHostFragmentss and the according NavControllers.

An alternative which solves 2, but not 1 is to specifically inject stuff provided by the parent Fragment/Activity within the (child) Fragment module. Thus we can omit any potential overrides as we don't want them in the Fragment anyway:

val myFragmentModule = createModule {

  bind<MyDependency> { singleton { (get<Fragment>(SUPPORT_FRAGMENT).activity as KatanaTrait).injectNow() }
}

But this somehow feels a bit strange as well as I suddenly exactly define from where I want to get my dependencies supplied. That part just feels more ioc with the dependsBy approach.

Any best practises here?

@svenjacobs
Copy link
Member

Hi @ubuntudroid, can you maybe provide a small example project reproducing this issue? It's easier for me to "think" in code :)

@svenjacobs svenjacobs self-assigned this Feb 1, 2019
@svenjacobs svenjacobs added the question Further information is requested label Feb 1, 2019
@svenjacobs
Copy link
Member

In the meantime I tried it out myself in the example application. Please have a look here and especially here.

Basically it boils down to the situation mentioned in your first point. The Component must be initialized with by lazy because it has a dependency to the parent Activity which is not available when the Fragment instance is created. Fields then cannot be injected via by component.inject() because this would initialise the component too early. So dependencies must be injected more or less manually in onActivityCreated() or any other lifecycle method that is called after the Activity was created.

I know this is not ideal. Maybe you, I or someone else comes up with a better idea?

@svenjacobs svenjacobs added the good first issue Good for newcomers label Feb 1, 2019
@svenjacobs
Copy link
Member

Regarding your point with Glide. If you configure Glide instances in modules so that they are tied to the Activity or Fragment and you have one binding in the Activity's module and one in the Fragment's module then yes, you have to use named bindings or else you have overrides. This is by design. When you request a Glide instance in your Fragment, which depends on the Activity component, then Katana wouldn't know if you want the Activity or the Fragment instance.

@svenjacobs
Copy link
Member

If you don't want to delegate certain dependencies to child components, like for example a Glide binding, what you can do of course is to create a private child component for your current Activity or Fragment, which contains the Glide binding through a separate module. You will then use this component for injection whereas the public component is still used for "inheritance".

class MainActivity : AppCompatActivity(),
                     KatanaTrait {

  override val component = createComponent(
    modules = listOf(...),
    dependsOn = listOf(...)
  )

  private val myComponent = createComponent(
    modules = listOf(createGlideModule(this)),
    dependsOn = listOf(component)
  }

  private val glide: GlideApp by myComponent.inject()
}

I'm thinking of maybe providing a simplified syntax for creating child components, something like

val childComponent = component.plus(listOf(childModule))
// or
val childComponent = component + listOf(childModule)

@ubuntudroid
Copy link
Author

Wow, that was a quick and super extensive response! 😮

First of all thanks for the Fragment sample code - that really helps a lot in determining how the canonical way of injecting with Katana is. 🙏

Further musings regarding your suggestions/samples and my own previous statements:

Thinking more about what by lazy semantically means in Kotlin ("initialise only when needed") I guess lateinit and component creation in onCreate() would be the proper thing to do here as usually we will need the component down the line (especially given that also as per your Fragment sample code the injects are performed using lateinit and assignments in onCreate()).

Unfortunately lateinit doesn't work as component is an override. That leaves us with to alternatives for more consistency here:

  1. Provide stuff bit by bit in the Fragment module as outlined in the second sample in my question. This however would somewhat run against ioc as I now have to exactly know where something is provided and it can also become quite cumbersome if I need to fetch a dependency from let's say the parent fragment of my parent fragment etc..
  2. To make things more consistent we could also use by lazy { injectNow() } (or a by lazyInject() alias) for the injected properties - then most things would work as expected (although we could possibly run into the situation that we try to access the property for the first time AFTER the fragment has been detached, then we would not be able to create the component anymore).

Therefore I guess your "lazy component" + "lateinit injected properties" + "depends On" approach seems to be the only feasible solution here for now. I still don't particularly like that part of katana to be honest, maybe I'll come up with another solution after using it a bit more.

@ubuntudroid
Copy link
Author

Btw, AFAIK you could remove the component.injectNow() from the sample fragment code and replace it with a simple injectNow(), but I guess you did that for clarity reasons.

@svenjacobs
Copy link
Member

Unfortunately lateinit doesn't work as component is an override.

Actually this is possible with override lateinit var component: Component 🙂 By the way you are not forced to use KatanaTrait. This is just an interface that provides a few handy extension functions.

I must admit that I'm not using Fragments in the project that I created Katana for. I'm using Conductor. Conductor's Controllers have a much simpler lifecycle and depending on the Katana component of the parent Activity works like a charm.

But I think we'll find a satisfying solution for Fragments, too.

@ubuntudroid
Copy link
Author

Oh wow, I should have tried overriding myself. 🤦‍♂️ I was so sure that it would not work - sorry for that!

I see your point - using Fragments certainly makes this a bit more complicated than with a purely View based app. There are different reasons why we keep on using Fragments (one of those is: we are using Google's Architecture Components which work nicely with Fragments).

I'll keep on experimenting with the lateinit approach and get back once I have a better idea of whether we can solve all of our use cases with that.

@svenjacobs
Copy link
Member

@ubuntudroid I updated the second Fragment example with an alternative approach. Here I created a Container class that holds all dependencies of the Fragment. As a result dependency resolution is again delegated to the Katana module where it should be. Only one dependency must then be injected manually into the fragment.

I'm thinking of maybe providing a simplified syntax for creating child components

Also version 1.2.7 of Katana contains the new syntax as you can see in the example.

@ubuntudroid
Copy link
Author

@svenjacobs Thanks for including the new syntax! 😍

While I like the idea of resolving dependencies in the module, I am a bit unsure whether I really like the container approach, especially the need to now call container.dependency to get hold of that dependency, but that might be just cosmetics.

Regardless of that, how would you handle potential overrides with that approach? Let's take the common case of a parent Fragment + child Fragment again. Both specify createSupportFragmentModule() in their module list. As that eventually ends up in the component you would end up with an override for SUPPORT_FRAGMENT and SUPPORT_FRAGMENT_CONTEXT in the child Fragment which depends on the parent Fragment component. How would one resolve this here?

@svenjacobs
Copy link
Member

Regardless of that, how would you handle potential overrides with that approach?

Dependencies that should be "inherited" are put into the public component whereas everything that should not be inherited or could cause overrides is put into a private component. See my other comment. With the new syntax this is even easier now ;)

@ubuntudroid
Copy link
Author

ubuntudroid commented Feb 4, 2019

@svenjacobs thanks for the clarification - I think I got that now. 🤓

Will hopefully be able to play around a bit with it tomorrow and come back with my findings.

The only thing that feels strange already is that we now have that primary first-class component from the KatanaTrait interface which allows us to use all those neat inject extensions and the ugly private component where we have to go with privateComponent.inject(). As this probably is a pretty common use case why not also have a PrivateKatanaTrait (or LocalKatanaTrait?) interface with a protected privateComponent which allows to injectPrivate(), injectNowPrivate() etc.? That might help unify usage in this use case while still retaining API clarity.

@svenjacobs
Copy link
Member

As this probably is a pretty common use case why not also have a PrivateKatanaTrait (or LocalKatanaTrait?) interface with a protected privateComponent which allows to injectPrivate(), injectNowPrivate() etc.?

Unfortunately interfaces cannot declare private or protected properties. Everything must be public. Of course I could create a DualKatanaTrait or something like that which declares two components but then again both components are public which renders the public/private component concept void.

@ubuntudroid
Copy link
Author

Ah yeah, of course, protected doesn't work in interfaces - what about switching the interface to an abstract class instead?

@ubuntudroid
Copy link
Author

Disregard that - as we have no multi-inheritance this would not work either in most classes and make the whole library a lot less flexible. 🤔

@svenjacobs
Copy link
Member

as we have no multi-inheritance this would not work either in most classes and make the whole library a lot less flexible

I was just about to say that 🙂

@ubuntudroid
Copy link
Author

So I think we now have a few working solutions for the problem mentioned in this issue. Someone will probably come up with something even better in the future, but for now I guess we can close the issue.

Thanks a million for your super active support, mate! 🔝 🙏

@svenjacobs
Copy link
Member

Glad I could help 🙂 If you have any further questions feel free to create a new issue or contact me via Slack. Looking forward to your opinion when you're using Katana in your production application 😎

@svenjacobs
Copy link
Member

In version 1.2.8 of Katana I added KatanaFragment and KatanaFragmentDelegate to the Android artifact. These utility classes should simplify usage of Katana with Fragments a bit. Also see the updated demo application.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Good for newcomers question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants