Feature/analyse dependencies graph by condesales · Pull Request #36 · ataulm/muvi

condesales · 2019-11-21T14:47:08Z

This is a proposal on how to handle the dependency between modules.
I used Jake Wharton's Gradle script with some minor changes to plot the dependency graph to help visualize it.

brew install graphviz
./gradlew projectDependencyGraph

Original Graph

Proposed Graph

The changes mainly follow the idea of using dependencies as api instead of implementation wherever I thought it made sense.

The onion diagram from Clean Arch always has the data layer as a superset of domain. By using that idea we would expect that when adding data as a dependency you would have access to its domain
I applied the same logic to the cache/remote "data stores" (not sure how you call them)

From the original graph, we could also see that :design_library and :navigation modules are all accessed by the end-feature modules :feed, :actor_details and :film_details.
I believe the whole idea of a :core module is to share all the shared dependencies between modules so moved those as api dependencies there.

One interesting case is for the :letterboxd_api. That module theoretically should only be accessed by remote modules but we need it as a dependency to :core because we need to build the dagger graph and provide it as @Singleton. Perhaps by using @Reusable we could achieve the same outcome?

It's important to be intelligent about what goes into :core as dependencies. Only those that are shared across all features should go there as we don't want to provide dependencies that are not needed by features. This could be a big impact on dynamic features.

I'm going to leave some comments on specific lines to explain some specific decisions, but the above summarises the whole idea.

I'd love to hear from you the thought process behind the original graph dependencies setup. I dived into this specifically because I was curious to see how other people have been tackling the same problem, so thanks for sharing!

… coming up with something else similar

…that

…ation

condesales · 2019-11-21T14:52:46Z

    implementation libraries.kotlinCoroutinesCore
    implementation libraries.kotlinStdLib
+
+    kapt libraries.daggerCompiler


It's always a good practice to add the dagger compiler to Gradle modules that define Dagger modules or have classes annotated with @Inject constructor()
it wouldn't happen in the current project setup, but in some cases when sharing a Gradle module you might get the Factory of that class only created by the module that uses it as a dependency and you could get multiple instances of the same Factory generated, causing a compilation issue :)

See Zac Sweer's blog post for more information on this:
https://www.zacsweers.dev/dagger-party-tricks-refactoring/

The trick here is simple: any project you have constructor injection, you should run the dagger compiler over it too.

condesales · 2019-11-21T14:53:46Z

 apply plugin: 'kotlin-kapt'

 dependencies {
-    implementation project(':actor_detail_data')


dependency was inverted
"_data modules know about the "data stores", not the other way around

Interesting that remote depended on data before this change. Usually we try to avoid the feature knowing about where data is coming from (i.e. remote or cache), so this change makes sense to me.

Is it possible for remote to be implementation instead of api so that the implementation detail doesn't get leaked to the feature?

That's a good shout.
I think I tried that before and it didn't work initially but I could give it another go with a different approach this time

condesales · 2019-11-21T14:56:05Z

@@ -1,4 +1,4 @@
-package com.muvi.actor_detail_data
+package com.muvi.actor_detail_remote


moved the class to invert the dependency

condesales · 2019-11-21T15:00:53Z

-            AppModule::class,
-            LetterboxdApiModule::class
-        ]
+    modules = [


sorry about this. Maybe the coding style could be added to the project at some point? 😬

condesales · 2019-11-21T15:01:33Z

+    }
+}
+
+private fun buildConfig(): ApiConfig {


This was the way I found to keep :letterboxd_api as a kotlin-only module

condesales · 2019-11-21T15:03:36Z

 apply plugin: 'kotlin-kapt'

 dependencies {
-    implementation project(':base_data')


couldn't really see the benefit of :base_data so I deleted it 😅

condesales · 2019-11-21T15:04:47Z

@@ -1,8 +1,10 @@
-apply plugin: 'kotlin'
+apply plugin: 'com.android.library'


had to be transformed into an android library module so it can depend on :feed_cache

condesales · 2019-11-21T15:08:47Z

 import dagger.Provides

-@Module
+@Module(includes = [


this helps to simplify the dagger graph as well

jamieadkins95 · 2019-11-21T17:22:15Z

The onion diagram from Clean Arch always has the data layer as a superset of domain. By using that idea we would expect that when adding data as a dependency you would have access to its domain

A nice way of thinking about this is that domain is part of the public API of data, since when you go to data to fetch a list of films, it gives you back some domain objects. You can't use data without also including domain, therefore depending on data should also give you domain.

jamieadkins95 · 2019-11-21T17:28:32Z

-    implementation project(':letterboxd_api')
+    api project(':letterboxd_api')
+    api project(':navigation')
+    api project(':design_library')


I'm not sold on core depending on design_library. You may build a feature that has completely bespoke UI elements that doesn't need anything from design_library.

This would be one of the first things I'd cut from core if I needed to save space for a 4MB instant app

SAGARSURI · 2020-01-19T16:45:30Z

@condesales can you share the script or process to generate the DI graph?

condesales · 2020-01-20T00:05:53Z

@SAGARSURI
It's in the PR
https://github.com/ataulm/muvi/pull/36/files#diff-731c4a6982ed61c6516c58a09d9456af

SAGARSURI · 2020-01-20T01:35:53Z

So after looking at the script. I believe I need to update the module names in the rank section? @condesales

SAGARSURI · 2020-01-20T01:37:11Z

Just curious, When will this be getting merged? @jamieadkins95

condesales · 2020-01-20T09:57:41Z

So after looking at the script. I believe I need to update the module names in the rank section? @condesales

Yeah, unfortunately, to get a good plot we had to hardcode some of the prefixes/suffixes of the names of the modules.

Felipe Conde added 10 commits November 20, 2019 11:07

adding dependency graph to repository

318691e

trying to simplify the gradle dependency graph for actor modules

e060706

using same idea to simplify the gradle dependency graph for films

177bc6d

idea used to simplify gradle graph doesn't really work well for feed.…

8207a89

… coming up with something else similar

we need to pass more info than just debuggable, lets use a class for …

cd0f43e

…that

centralise shared dependencies into :core

efc17c9

making improvements to the graph plot so we can get a better visualiz…

7e49305

…ation

spelling

1be7737

adding credit to the script

ba5d208

revert .gitignore changes

f1bfcf9

condesales commented Nov 21, 2019

View reviewed changes

jamieadkins95 reviewed Nov 21, 2019

View reviewed changes

condesales marked this pull request as ready for review November 27, 2019 10:41

		@@ -1,4 +1,4 @@
		package com.muvi.actor_detail_data
		package com.muvi.actor_detail_remote

		@@ -1,8 +1,10 @@
		apply plugin: 'kotlin'
		apply plugin: 'com.android.library'

Conversation

condesales commented Nov 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamieadkins95 commented Nov 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SAGARSURI commented Jan 19, 2020

Uh oh!

condesales commented Jan 20, 2020

Uh oh!

SAGARSURI commented Jan 20, 2020

Uh oh!

SAGARSURI commented Jan 20, 2020

Uh oh!

condesales commented Jan 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants