Should we enforce the Dependency Inversion Principle (DIP)?

[ChristopherPHolder]

Motivation

This pattern promotes decoupling, making the system easier to maintain, scale, and modify. It also facilitates unit testing since you can mock the abstractions.

1. High-level modules should not depend on low-level modules. Both should depend on abstractions.
2. Abstractions should not depend on details. Details should depend on abstractions.

Implementation Overview

We can achieve this be making sure the Core Module and the CLI both depend on the abstractions in the Models Module. This means we would move the abstraction there and that module will actually dictate the behavior of the system.

(In the diagram you can see the the dependency between CLI and Core is a doted line. This is because the core module should implement the abstraction in Models and therefore CLI actually depends on Models and Core could be replace with anything that satisfies the abstractions in models)

By making sure the abstraction are in the Models Module and that both the Core Module and the CLI depend on these abstraction we essentially inverting the dependency such that higher-level and lower-level modules both depend on an abstraction rather than the higher-level module depending on the details of a lower-level module.

Because the Core Module handles implementing the abstraction from the Models Module, the CLI does not require depending on the Utils Module. Therefore I suggest we enforce module boundaries on the CLI so that it can only depend on Core and Models.

This would also mean that the Plugins Modules do not require the Core Module , as all abstraction needed to create a plugin are located in the Models Module and all the function that would help the plugin authors create these plugins are located in the Utils Module.

As part of this i would also suggest we enforce module bounties so that the Plugins Modules can only depend on Utils and Modules. Potentially in the future we can also have testing utils and the boundaries can be extended to include that module.

Because Utils depends on Models and and core implements the logic related to the CLI execution. We should be very mindful of what can be imported in the module. For example it should not import yargs as that should belong in the Core Module, nor should it import anything that is specific to a plugin, like lighhouse or any eslint internal.

TODOS

• Enforce Module Boundaries with NX

  • CLI can only depend on Models and Core
  • Plugins can only depend on Models and Utils

• Restrict imports in Utils and Plugins

  • Utils and Plugins cannot import Yargs

• Move an create abstraction in Models

  • As we move Core out of Utils we should move the abstractions to Models

  Related to: Move core logic into new "core" package #73

[BioPhoton]

Nice issue!

Could you please add examples for the suggestions so we can discuss it better.

Also yargs I would see as dependency only in the cli package not in core.

From the dependencies see it like this:

• cli - all terminal, prompting
• core - execution, file system, server
• models - parsing, typing
• utils - helper, parser, errors

flowchart LR;
  
    subgraph npm
    models-->zod;
    utils-->simple-git;
    cli-->yargs;
    cli-->chalk;
    cli-->uicli;
    plugin-*-->eslint;
    plugin-*-->lighthouse;

    subgraph code-pushup
    utils-->models;
    utils-->|X|cli;
    cli-->models;
    cli-->core;
    core-->models;
    core-->utils;
    plugin-*-->utils;
    plugin-*-->models;
    end


    subgraph integrator
    gh-action-->models;
    gh-action-->core;
    nx-validators-plugin-->models;
    nx-validators-plugin-->core;
    end


    end
   click models "https://github.com/flowup/quality-metrics-cli/tree/main/packages/models" "view source"

Loading

docs for the diagram

WDYS?

Regarding the todos:

utils and eslint-plugin cannot import yargs

Here I would say that Plugins import yargs or any other lib they want.

As we move Core out of Utils we should move the abstractions to Models

Could you please give some examples here

[ChristopherPHolder]

Also yargs I would see as dependency only in the cli package not in core.

I agree. I might also see it as a dependency in models but that is just to simplify typing.

Here I would say that Plugins import yargs or any other lib they want.

If the plugins import Yargs they will implement their own abstraction and not reuse the ones in models.

We probably want to have the cli handle the prompts, the param parsing and the logging. The plugins can use these through the utils. For example the logger can be a singleton service streaming values. If it’s exposed through utils but instantiated in cli and called in core and plugins then we can handle its behavior however we want. If we want to only execute on verbose or have different levels or logging that acceptable. If we want the logs to be persisted (great for testing and debugging) then the we can implement that in this logging service.

[matejchalk]

Fully agree 🙂 The Mermaid diagram illustrates it very well, I think 👍

Move an create abstraction in Models
As we move Core out of Utils we should move the abstractions to Models

@ChristopherPHolder Not sure what you mean by "abstractions" here exactly. The models package already contains Zod schemas and inferred types. Is there something else you think we should be adding to models?