Skip to content

Conversation

@aitorvs
Copy link
Collaborator

@aitorvs aitorvs commented Feb 22, 2021

Task/Issue URL: https://app.asana.com/0/414730916066338/1199916263225064/f
Tech Design URL:
CC:

Description:
We've been using Dagger Android for a while and it has been good help. We've also seen some drawbacks moving forward

  • it is not easy to replace dependencies for testing, forcing us to create a TestComponent, custom TestRunner and a custom TestApplication
  • this also comes with some other problems like circular dependencies when splitting the codebase into multiple modules
  • multi (gradle) module is harder to get right with just Dagger and Dagger-Android and we want to start splitting the app little by little

This PR:

  • Adds Anvil and remove dagger-android annotation processor
  • Creates a gradle module :di that is home to the Dagger components marker (scope) annotations
  • Removes the TestApplication and TestComponent
  • Adds a a bunch of binding classes under com.duckduckgo.app.di.component
  • these files replace the now removed AndroidBindingModule.kt that used the dagger-android annotation processor (DAAP)
  • DAAP can't work with Anvil because we need to replace the @Component and @SubComponent with @MergeComponent and @MergeSubComponent correspondingly and so we can't do that for the DAAP-generated code.
  • the codee in those files is pretty much the code generated by DAAP
  • the con is that we need to do it ourselves now, tho we can easily do this by either copy/paste/replace and/or a file template in AS (I did with a file template)
  • the pro is that it is now very clear what is under the hood, and so DI errors are less obscure now (hopefully)

Note:

  • full build -> 1min40s
  • incremental build -> 20s

Steps to test this PR:

  1. App compiles
  2. All tests pass
  3. updgrade/install the app from this branch
  4. verify that every entry that was in the AndroidBindingModule.kt has now a dedicated file in the package com.duckduckgo.app.di.component
  • eg. BrowserActivity -> BrowserActivityComponent.kt

Internal references:

Software Engineering Expectations
Technical Design Template

@aitorvs aitorvs force-pushed the feature/aitor/anvil branch from 71f21be to 18f70d9 Compare February 22, 2021 22:20
@Module
@ContributesTo(
scope = AppObjectGraph::class,
replaces = [AppConfigurationDownloaderModule::class]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how we replace dagger modules for testing with Anvil. No more TestComponent needed

Comment on lines +47 to +48
@MergeComponent(
scope = AppObjectGraph::class,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MergeComponent is the anvil-equivalent to dagger @Component. This annotation is what anvil uses to generate the dagger components under the hood.
The scope is not be confused to eg. ActivityScope or Singleton scope-annotations. It simply is a marker class that identifies the dagger component, so that we can later provide modules/dependencies without needing to get access to the dagger component type

Comment on lines +100 to +102
// accessor to Retrofit instance for test only only for test
@Named("api")
fun retrofit(): Retrofit
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should find a better way to do this. This is needed by the ATB e2e tests and it was done in the same manner in the now removed TestComponent.
I will think about this and prob tackle it in a different PR

}
dependencies {
classpath 'com.android.tools.build:gradle:4.1.0'
classpath 'com.android.tools.build:gradle:4.1.1'
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took the opportunity to bump a patch version here

@aitorvs aitorvs force-pushed the feature/aitor/anvil branch from 18f70d9 to 92a984d Compare February 22, 2021 22:31
@aitorvs aitorvs force-pushed the feature/aitor/anvil branch from 92a984d to 200ca5d Compare February 23, 2021 09:54
@marcosholgado marcosholgado self-requested a review February 23, 2021 11:06
@marcosholgado marcosholgado self-assigned this Feb 23, 2021
Copy link
Contributor

@marcosholgado marcosholgado left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job @aitorvs! 👏

  • Made sure we replaced everything within the AndroidBindingModule.
  • Tested updating the app, onboarding, bookmarks, fire button, fireproof, location dialog, pixels, tabs, find in page, privacy dashboard, grade animation, broken sites, icon change and notifications.

@aitorvs aitorvs merged commit c13aceb into develop Feb 23, 2021
@aitorvs aitorvs deleted the feature/aitor/anvil branch February 23, 2021 12:34
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.

2 participants