Dependency graphs

[SimonMarquis]

First of all, let's ackowledge the initial work of @mmoczkowski in #310 for the very nice looking graphs, but it seems like we won't be able to get this tool open-sourced 😢

So, here is an attempt to improve the current state of dependency graphs in this project.
Let's take :feature:topic as an example in this discussion as it contains a good amount of dependencies.
In this discussion, I'll list some important questions that I think we need to answer before pushing the first PR, as well as potential solutions with examples.

The code to generate these graphs is ~100 loc (and runs in a few milliseconds) and is currently designed as a custom convention plugin in build-logic, registering a task that dumps these graphs for each modules. But the implementation can be changed depending on what we need in the end.

[SimonMarquis]

Mermaid vs Graphviz (dot)

• Mermaid

  • GitHub automatically renders the source content in Markdown files.

  • IntelliJ has support for it

    (not officially compatible with Android Studio yet, https://issuetracker.google.com/issues/159933628)

    

  • "interactive" in the browser zoom/pan/etc.

• Graphviz
  • dot layout engine seems to be smarter on large graphs: the amount of crossings is usually smaller
• Both
  • can be rendered to png/svg
  • extensible customization
  • Online web app with link including the source

---

Example without any customization

Mermaid

Edit

graph LR

  subgraph :core
    :core:analytics[analytics]
    :core:common[common]
    :core:data[data]
    :core:database[database]
    :core:datastore[datastore]
    :core:datastore-proto[datastore-proto]
    :core:designsystem[designsystem]
    :core:model[model]
    :core:network[network]
    :core:notifications[notifications]
    :core:ui[ui]
  end
  subgraph :feature
    :feature:topic[topic]
  end

  :core:data ---> :core:analytics
  :core:data ---> :core:common
  :core:data ---> :core:database
  :core:data ---> :core:datastore
  :core:data ---> :core:network
  :core:data ---> :core:notifications
  :core:database ---> :core:model
  :core:datastore ---> :core:common
  :core:datastore ---> :core:datastore-proto
  :core:datastore ---> :core:model
  :core:network ---> :core:common
  :core:network ---> :core:model
  :core:notifications ---> :core:common
  :core:notifications ---> :core:model
  :core:ui ---> :core:analytics
  :core:ui ---> :core:designsystem
  :core:ui ---> :core:model
  :feature:topic ---> :core:data
  :feature:topic ---> :core:designsystem
  :feature:topic ---> :core:ui

Loading

Graphviz

Edit

[yschimke]

Mermaid seems the better option here given github and Intellij support. I'd vote for adopting tools requiring less additional investment in tooling by the project.

[yschimke]

The mermaid example here also look more modern and crisp.

[SimonMarquis]

api vs implementation

Should we distinguish api and implementation configuration? And if so, how?
I personnaly think it can be usefull to show this information, because exposing dependencies as api when it's not needed can have consequences.

I see at least 2 easy options to surface this information (and I prefer solution 2):

Solution 1: Add a label on every link

Mermaid

Edit

graph LR
  :core:analytics[analytics]
  :core:common[common]
  :core:data[data]
  :core:database[database]
  :core:datastore[datastore]
  :core:datastore-proto[datastore-proto]
  :core:designsystem[designsystem]
  :core:model[model]
  :core:network[network]
  :core:notifications[notifications]
  :core:ui[ui]
  :feature:topic[topic]

  :core:data --->|implementation| :core:analytics
  :core:data --->|api| :core:common
  :core:data --->|api| :core:database
  :core:data --->|api| :core:datastore
  :core:data --->|api| :core:network
  :core:data --->|implementation| :core:notifications
  :core:database --->|api| :core:model
  :core:datastore --->|implementation| :core:common
  :core:datastore --->|api| :core:datastore-proto
  :core:datastore --->|api| :core:model
  :core:network --->|api| :core:common
  :core:network --->|api| :core:model
  :core:notifications --->|implementation| :core:common
  :core:notifications --->|api| :core:model
  :core:ui --->|api| :core:analytics
  :core:ui --->|api| :core:designsystem
  :core:ui --->|api| :core:model
  :feature:topic --->|implementation| :core:data
  :feature:topic --->|implementation| :core:designsystem
  :feature:topic --->|implementation| :core:ui

Loading

Graphviz

Edit

Solution 2: Style configurations differently

Dashed links for implementation (aka private dependency) vs plain links for api (aka public dependency).

Mermaid

Edit

graph LR
  :core:analytics[analytics]
  :core:common[common]
  :core:data[data]
  :core:database[database]
  :core:datastore[datastore]
  :core:datastore-proto[datastore-proto]
  :core:designsystem[designsystem]
  :core:model[model]
  :core:network[network]
  :core:notifications[notifications]
  :core:ui[ui]
  :feature:topic[topic]

  :core:data -.-> :core:analytics
  :core:data ---> :core:common
  :core:data ---> :core:database
  :core:data ---> :core:datastore
  :core:data ---> :core:network
  :core:data -.-> :core:notifications
  :core:database ---> :core:model
  :core:datastore -.-> :core:common
  :core:datastore ---> :core:datastore-proto
  :core:datastore ---> :core:model
  :core:network ---> :core:common
  :core:network ---> :core:model
  :core:notifications -.-> :core:common
  :core:notifications ---> :core:model
  :core:ui ---> :core:analytics
  :core:ui ---> :core:designsystem
  :core:ui ---> :core:model
  :feature:topic -.-> :core:data
  :feature:topic -.-> :core:designsystem
  :feature:topic -.-> :core:ui

Loading

Graphviz

Edit

---

PS: I also choose to "hide" implementation links when there is already a "higher visibility" api link, which helps with visibility.
PPS: I tried "coloring" the link, but I was not convinced it helped understanding the difference between the two configuration.

[yschimke]

I find the label to be noisy and distracts me. So I prefer the styling.

Since each graph is targetted at a specific module such as the topic feature, I wonder if we can also style the boxes to indicate which are API level for the target.

My reasoning would be that it shows the transitive nature. topic has no API dependencies, so for this module, everything is implementation.

[yschimke]

Or potentially only show the API dependencies? Would less information convey the message more effectively?

[SimonMarquis]

I find the label to be noisy and distracts me. So I prefer the styling.

I agree. We could also define the semantics once in this project to avoid confusion about colors/styles/etc.

I wonder if we can also style the boxes to indicate which are API level for the target.

To me, this would not make sense. Links represents relationships, boxes represents modules.
Styling boxes should be based on the module itself, and not on the relationship it has over other boxes.
Styling

Or potentially only show the API dependencies? Would less information convey the message more effectively?

IMO, restricting visiblity to only API dependencies would hide too many information that I think are worth mentioning. If for example, during a PR, unrelated (and unnecessary) modules get added as impl dependencies, I want to make them appear in the graph and reject them.
Having impl dependencies is not "free". While they are hidden from the consumer side, they still have an impact on compilation etc.

[SimonMarquis]

Grouping by "scope" (parent module)

I think it looks great, and helps with the overall legibillity.

Mermaid

Edit

graph LR

  subgraph :core
    :core:analytics[analytics]
    :core:common[common]
    :core:data[data]
    :core:database[database]
    :core:datastore[datastore]
    :core:datastore-proto[datastore-proto]
    :core:designsystem[designsystem]
    :core:model[model]
    :core:network[network]
    :core:notifications[notifications]
    :core:ui[ui]
  end
  subgraph :feature
    :feature:topic[topic]
  end

  :core:data -.-> :core:analytics
  :core:data ---> :core:common
  :core:data ---> :core:database
  :core:data ---> :core:datastore
  :core:data ---> :core:network
  :core:data -.-> :core:notifications
  :core:database ---> :core:model
  :core:datastore -.-> :core:common
  :core:datastore ---> :core:datastore-proto
  :core:datastore ---> :core:model
  :core:network ---> :core:common
  :core:network ---> :core:model
  :core:notifications -.-> :core:common
  :core:notifications ---> :core:model
  :core:ui ---> :core:analytics
  :core:ui ---> :core:designsystem
  :core:ui ---> :core:model
  :feature:topic -.-> :core:data
  :feature:topic -.-> :core:designsystem
  :feature:topic -.-> :core:ui

Loading

Graphviz

Edit

[yschimke]

+1 this really helps and carries over this feature from https://github.com/android/nowinandroid/pull/310/files#diff-75afa4acc3e5f9752389155b4c953eb63c33fa8d3c676ac235d858cd3e19d722

[yschimke]

This is a core part of the modularization doc, so perhaps we should focus on this and also include matching colours?

https://github.com/android/nowinandroid/blob/main/docs/ModularizationLearningJourney.md

[SimonMarquis]

Module types

I feel like rendering pure (lightweight) Kotlin JVM projects differently from Android (heavy) projects is a good addition.

• Android #3DDC84
• JVM #7F52FF

Mermaid

Edit

graph LR

  subgraph :core
    :core:analytics[analytics]:::android
    :core:common[common]:::jvm
    :core:data[data]:::android
    :core:database[database]:::android
    :core:datastore[datastore]:::android
    :core:datastore-proto[datastore-proto]:::android
    :core:designsystem[designsystem]:::android
    :core:model[model]:::jvm
    :core:network[network]:::android
    :core:notifications[notifications]:::android
    :core:ui[ui]:::android
  end
  subgraph :feature
    :feature:topic[topic]:::android
  end

  :core:data -.-> :core:analytics
  :core:data ---> :core:common
  :core:data ---> :core:database
  :core:data ---> :core:datastore
  :core:data ---> :core:network
  :core:data -.-> :core:notifications
  :core:database ---> :core:model
  :core:datastore -.-> :core:common
  :core:datastore ---> :core:datastore-proto
  :core:datastore ---> :core:model
  :core:network ---> :core:common
  :core:network ---> :core:model
  :core:notifications -.-> :core:common
  :core:notifications ---> :core:model
  :core:ui ---> :core:analytics
  :core:ui ---> :core:designsystem
  :core:ui ---> :core:model
  :feature:topic -.-> :core:data
  :feature:topic -.-> :core:designsystem
  :feature:topic -.-> :core:ui

classDef android fill:#3ddc84,stroke:#fff,stroke-width:2px,color:#fff;
classDef jvm fill:#7F52FF,stroke:#fff,stroke-width:2px,color:#fff;

Loading

Graphviz

Edit

[yschimke]

I's suggest deferring this until a possible future KMP Now in Android. While it's Android only, I don't think this separate is as critical.

[SimonMarquis]

I don't feel like deferring it until a potential KMP version is landed will be beneficial in this context, as we won't "discover" rules in the future that will invalidate this initial convention. The project already contains JVM modules. And it is not a huge effort to style boxes based on the associated module type.

[SimonMarquis]

Being an Android app doesn't mean we should make all our modules "Android" modules.
This has a big negative impact on the overall build.
And I think it is important for Android devs to understand this, or at least to be aware of it.

[SimonMarquis]

Focus the "target" module

Not sure if this will be really helpful since it will always be the left-most node.

Mermaid

Edit

graph LR

  subgraph :core
    :core:analytics[analytics]:::android
    :core:common[common]:::jvm
    :core:data[data]:::android
    :core:database[database]:::android
    :core:datastore[datastore]:::android
    :core:datastore-proto[datastore-proto]:::android
    :core:designsystem[designsystem]:::android
    :core:model[model]:::jvm
    :core:network[network]:::android
    :core:notifications[notifications]:::android
    :core:ui[ui]:::android
  end
  subgraph :feature
    :feature:topic[topic]:::android-focus
  end

  :core:data -.-> :core:analytics
  :core:data ---> :core:common
  :core:data ---> :core:database
  :core:data ---> :core:datastore
  :core:data ---> :core:network
  :core:data -.-> :core:notifications
  :core:database ---> :core:model
  :core:datastore -.-> :core:common
  :core:datastore ---> :core:datastore-proto
  :core:datastore ---> :core:model
  :core:network ---> :core:common
  :core:network ---> :core:model
  :core:notifications -.-> :core:common
  :core:notifications ---> :core:model
  :core:ui ---> :core:analytics
  :core:ui ---> :core:designsystem
  :core:ui ---> :core:model
  :feature:topic -.-> :core:data
  :feature:topic -.-> :core:designsystem
  :feature:topic -.-> :core:ui

classDef android-focus fill:#3ddc84,stroke:#000,stroke-width:4px,color:#fff;
classDef android fill:#3ddc84,stroke:#fff,stroke-width:2px,color:#fff;
classDef jvm fill:#7F52FF,stroke:#fff,stroke-width:2px,color:#fff;

Loading

Graphviz

Edit

[yschimke]

I agree with your reasoning. That said, would it be useful to also show incoming links to the module?

[SimonMarquis]

I think showing incoming links might makes leaf nodes even more confusing.
And the main information of incoming links will already be present in the global graph.
I personally would vote agains showing incoming links in module-targetted graphs.

[SimonMarquis]

Critical path

I don't think adding the critical paths (they often are many ) is really valuable here, and makes the graphs more complex than they need to.
It it certainly useful for larger projects with hundreds of modules, but here, when the height is around 4-5, it's easy to "detect" if something is wrong.

For example, here the critical height is 3, which makes almost everything painted red.

Mermaid

Edit

graph LR

  subgraph :core
    :core:analytics[analytics]:::android
    :core:common[common]:::jvm
    :core:data[data]:::android
    :core:database[database]:::android
    :core:datastore[datastore]:::android
    :core:datastore-proto[datastore-proto]:::android
    :core:designsystem[designsystem]:::android
    :core:model[model]:::jvm
    :core:network[network]:::android
    :core:notifications[notifications]:::android
    :core:ui[ui]:::android
  end
  subgraph :feature
    :feature:topic[topic]:::android
  end

  :core:data -.-> :core:analytics
  :core:data ---> :core:common
  :core:data ---> :core:database
  :core:data ---> :core:datastore
  :core:data ---> :core:network
  :core:data -.-> :core:notifications
  :core:database ---> :core:model
  :core:datastore -.-> :core:common
  :core:datastore ---> :core:datastore-proto
  :core:datastore ---> :core:model
  :core:network ---> :core:common
  :core:network ---> :core:model
  :core:notifications -.-> :core:common
  :core:notifications ---> :core:model
  :core:ui ---> :core:analytics
  :core:ui ---> :core:designsystem
  :core:ui ---> :core:model
  :feature:topic -.-> :core:data
  :feature:topic -.-> :core:designsystem
  :feature:topic -.-> :core:ui

classDef android fill:#3ddc84,stroke:#fff,stroke-width:2px,color:#fff;
classDef jvm fill:#7F52FF,stroke:#fff,stroke-width:2px,color:#fff;

linkStyle 2,3,4,5,6,7,8,9,10,11,12,13,17 stroke:red;

Loading

Graphviz

Edit

[yschimke]

How is the critical path applied to module dependencies?

Reading https://www.uber.com/en-GB/blog/crisp-critical-path-analysis-for-microservice-architectures/, I can see how it's important for networks given the critical path is time and sequencing, and relates to reliability.

But I suspect for a module diagram, the length of the path doesn't carry the same concerns. A couple of reasons:

1. If data depends on database (api) and database depends on datastore-proto (api), then it follows that data will depend on datastore-proto (api).
2. I think I'd often prefer my dependencies to be deeper rather than wider. My concern with data is that it has a broad set of dependencies.

[SimonMarquis]

But I suspect for a module diagram, the length of the path doesn't carry the same concerns.

Well, it is, but it is only a matter of parallelisation.

If for example you have an app with only these modules and this dependency chain:
:app → :feature1 → :core:domain → :core:utils-1 → :core:utils-2 → :core:utils-3 → :core:utils-4

then you won't be able to parallelize, as everything is on the dependency chain.
Having proper impl vs api configuration will help in case of cache invalidation, but the chain will remain for the main build.

I think I'd often prefer my dependencies to be deeper rather than wider.

I think both being deeper and being wider are not good for the overall parallelization.

My concern with data is that it has a broad set of dependencies.

I completely agree with you. NiA does not follow clean architecture (it has been discussed multiple times 😛 ), which means in this current architecture, the data layer has many responsibilities and therefore depends on many other modules.

[SimonMarquis]

Links renderer

There are many alternatives to the default layout rederer, making links sometimes easier to understand for larger graphs.

Seems like, ELK renderer does not support linkStyle, even though it was fixed at some point for flowcharts (mermaid-js/mermaid#4739, mermaid-js/mermaid#4813, mermaid-js/mermaid#4844)

Fixed in version 11: https://github.com/mermaid-js/mermaid/releases/tag/v11.0.0

see current version

info

Loading

Mermaid (elk)

Edit

%%{init:{'theme':'dark', 'flowchart': { 'defaultRenderer': 'elk' } }}%%

graph LR

  subgraph :core
    :core:analytics[analytics]:::android
    :core:common[common]:::jvm
    :core:data[data]:::android
    :core:database[database]:::android
    :core:datastore[datastore]:::android
    :core:datastore-proto[datastore-proto]:::android
    :core:designsystem[designsystem]:::android
    :core:model[model]:::jvm
    :core:network[network]:::android
    :core:notifications[notifications]:::android
    :core:ui[ui]:::android
  end
  subgraph :feature
    :feature:topic[topic]:::android
  end

  :core:data -.-> :core:analytics
  :core:data ---> :core:common
  :core:data ---> :core:database
  :core:data ---> :core:datastore
  :core:data ---> :core:network
  :core:data -.-> :core:notifications
  :core:database ---> :core:model
  :core:datastore -.-> :core:common
  :core:datastore ---> :core:datastore-proto
  :core:datastore ---> :core:model
  :core:network ---> :core:common
  :core:network ---> :core:model
  :core:notifications -.-> :core:common
  :core:notifications ---> :core:model
  :core:ui ---> :core:analytics
  :core:ui ---> :core:designsystem
  :core:ui ---> :core:model
  :feature:topic -.-> :core:data
  :feature:topic -.-> :core:designsystem
  :feature:topic -.-> :core:ui

classDef android fill:#3ddc84,stroke:#fff,stroke-width:2px,color:#fff;
classDef jvm fill:#7F52FF,stroke:#fff,stroke-width:2px,color:#fff;

Loading

Graphviz splines=ortho

Edit

[SimonMarquis]

test and androidTest configurations

Should we mix them with the "main" dependencies or should we add distinct graphs for that?
I feel like having them mixed makes it harder to reason about.
Also, if we want them in the same graphs, we should find a way to distinguish them, with either a label (as shown in the examples), a color (green like the IntelliJ convention), or something else?

Mermaid

Edit

graph LR

  subgraph :core
    :core:analytics[analytics]:::android
    :core:common[common]:::jvm
    :core:data[data]:::android
    :core:database[database]:::android
    :core:datastore[datastore]:::android
    :core:datastore-proto[datastore-proto]:::android
    :core:datastore-test[datastore-test]:::android
    :core:designsystem[designsystem]:::android
    :core:model[model]:::jvm
    :core:network[network]:::android
    :core:notifications[notifications]:::android
    :core:screenshot-testing[screenshot-testing]:::android
    :core:testing[testing]:::android
    :core:ui[ui]:::android
  end
  subgraph :feature
    :feature:topic[topic]:::android
  end

  :core:data -.-> :core:analytics
  :core:data ---> :core:common
  :core:data ---> :core:database
  :core:data ---> :core:datastore
  :core:data -.->|test| :core:datastore-test
  :core:data ---> :core:network
  :core:data -.-> :core:notifications
  :core:data -.->|test| :core:testing
  :core:database ---> :core:model
  :core:datastore -.-> :core:common
  :core:datastore ---> :core:datastore-proto
  :core:datastore -.->|test| :core:datastore-test
  :core:datastore ---> :core:model
  :core:datastore-test -.-> :core:common
  :core:datastore-test -.-> :core:datastore
  :core:designsystem -.->|test| :core:screenshot-testing
  :core:network ---> :core:common
  :core:network ---> :core:model
  :core:notifications -.-> :core:common
  :core:notifications ---> :core:model
  :core:screenshot-testing -.-> :core:designsystem
  :core:testing ---> :core:analytics
  :core:testing ---> :core:common
  :core:testing ---> :core:data
  :core:testing ---> :core:model
  :core:testing ---> :core:notifications
  :core:ui ---> :core:analytics
  :core:ui ---> :core:designsystem
  :core:ui ---> :core:model
  :core:ui -.->|androidTest| :core:testing
  :feature:topic -.-> :core:data
  :feature:topic -.-> :core:designsystem
  :feature:topic -.->|androidTest| :core:testing
  :feature:topic -.->|test| :core:testing
  :feature:topic -.-> :core:ui

classDef android fill:#3ddc84,stroke:#fff,stroke-width:2px,color:#fff;
classDef jvm fill:#7F52FF,stroke:#fff,stroke-width:2px,color:#fff;

Loading

Graphviz

Edit

[yschimke]

I'm not sure what I'd use this information for. There is definitely a risk of having too much information, so I'd vote for not showing them at all.

[dturner]

@SimonMarquis Just to let you know I haven't forgotten about this, just been busy with other things.

@yschimke FYI

[yschimke]

If we minimised the information in each module graph, is it easy to navigate between them by click?

[SimonMarquis]

Unfortunately, GitHub sanitizes links on mermaid graphs. Though I think it works on IntelliJ IDEA.

[yschimke]

Given https://github.com/android/nowinandroid/blob/main/docs/ModularizationLearningJourney.md, can we highlight any mistakes we should correct. Maybe there aren't any.

A feature module should have no dependencies on other feature modules.

But it would be interested to see layering violations if any.

[SimonMarquis]

If feel like this is not the responsibility of the graph, and since we want to have a violations in our graphs, this feature would never (or rarely) be seen.
But I agree with you, some patterns are violations that should not be merged.
What could be done instead is a distinct typological validation.
It can easily be hardcoded (instead of depending on an external library like modules-graph-assert that does other things) somewhere else in the build-logic, and reported as error during builds (could be its own issue on GitHub).