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

Dagger android #23

Merged
merged 34 commits into from
Apr 4, 2018
Merged

Dagger android #23

merged 34 commits into from
Apr 4, 2018

Conversation

alvarez-fabian
Copy link
Contributor

@alvarez-fabian alvarez-fabian commented Jul 24, 2017

Summary

Add support for for dagger and compatibility with the new changes in wolmo-core and wolmo-networking.This Pull Request adds breaking changes to the old implementations and we all should check if the changes can be adapted to other projects.

To better understand this changes I recommend to take a look to the official docs of dagger:

I'll try to explain how does this works and how we use wolmo-core and wolmo-networking.

Wolmo-Core and Wolmo-Networking provides the modules and some components to simplify the dependency injection of their classes or dependencies.
It's up to the project using those libraries to build a Component to use the modules and to inject the fragments and activities.

As explained in the docs you need to provide a @SubComponent to each Activity and Fragment to dagger to work. This is needed because dagger cannot inject dependencies on the constructor or at build time.

So this @SubComponent defines an android injector that will inject the Activity/Fragment at runtime in an specific lifecycle method:

  • onCreate on activities
  • onAttach on fragments.

Dagger will look for the injector at runtime and fail if it does not find it. You can look more about bindings in the multibinding docs.
You can see ViewPagerModule and ViewPagerSubComponent with examples of how to do that, that's the "hard" way.
If you don't need to customize or change anything in those modules and components you can use @ContributesAndroidInjector that will generate that code for you, you can see an example of that in InjectorModule. To avoid passing instances to a module, the component uses @BindsInstance.

AppComponent depends on NetworkingComponent to provide networking dependencies. The component needs to define a method that receives a NetworkingComponent to utilize it in the application. You can customize the arguments for the NetworkingComponent with a set of default methods, if you need further customization you can make your own AppNetworkingComponent using the modules provided with wolmo-networking or use your own.

You are free to organize the modules as you wish, but some recommended way could be:

  • Add in AppModule all the fragments and activities of the applications.
  • Add a new Module/SubComponent for each activity and then use @ContributesAndroidInjector for each fragment in that activity.

Package organization

For now the Components and generic modules are placed inside the package di.
Specific modules for some activities or fragments can be placed inside that "feature" package. (Like ViewPagerModule).

Tests

I recommend the use of AssertJ to write the tests. It provides a more fluent way to verify the output of the methods. I find it easier to understand and to verify than base JUnit or the Hamcrest Matchers.
Please tell me what do you think of this library.

I do not recommend the use of Powermock on big tests, it could create a bad practice. I only used it to mock android Log.

Also we are using mockito-inline instead of mockito-core, mockito-inline enables support for final classes mocks. It could add some drawbacks but in general I find it very stable.

TODO

We could add more examples on the tests, also we need to check and test if the dagger scopes are managed correctly.
In this code we use member injection and constructor injection as an example. We need to define which method we'll use as default.
We need to define a guide and a rule on how to configure this, for now I think that approach is the cleanest one.

@sbalay
Copy link

sbalay commented Aug 30, 2017

@emalamela @juanignaciomolina @Karamchi any comments? should we move forward with this?

@SpicyCactuar SpicyCactuar removed their request for review August 30, 2017 14:27
@SpicyCactuar SpicyCactuar removed their assignment Aug 30, 2017
@SpicyCactuar
Copy link
Contributor

@sbalay This is one of a series of three PRs which are related between them. Since it's a fundamental change, the idea was that Android team looked over each PR. I have already commented Wolox/wolmo-core-android#21 and still have to check Wolox/wolmo-networking-android#5. I'll leave this one to the others.

Besides that, one of the 3 Android sub-teams was tasked with reading these PRs. I don't remember which one.

@alvarez-fabian
Copy link
Contributor Author

@SpicyCactuar
Copy link
Contributor

SpicyCactuar commented Feb 5, 2018

@alvarez-fabian @juanignaciomolina @DemianIac @Karamchi This PR is open since July 27. We seriously need to merge it.

@alvarez-fabian Can you list the stuff that needs to happen in order to be ready?

@alvarez-fabian alvarez-fabian merged commit 5d065e7 into development Apr 4, 2018
@alvarez-fabian alvarez-fabian deleted the dagger-android branch April 4, 2018 19:14
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.

5 participants