Technical Debt: Module Organization

[austincondiff]

Our application's user interface is laid out similar to Xcode like so.

Our Modules folder doesn't quite reflect this. I've opened #821 and began revising Modules to reflect this in a PR, #823.

As I got into moving the TabBar from the main app into its own module, I encountered cyclical dependency declaration errors.

You can see what the code looks like in #831.

The problem was that TabBar depended on CodeEditUI which depended on WorkspaceClient because ToolbarBranchPicker and was in CodeEditUI. So I moved ToolbarBranchPicker into Git which seemed more appropriate but I ran into similar cyclical issues.

So @matthijseikelenboom and I thought about our architecture.

Because ToolbarBranchPicker is single-use, would it be more appropriate to put it in the main app?

But then again, our tab bar, navigators and inspectors are also single-use. By that logic we'd also move those into our main app.

But aren't each navigator and inspector eventually going to be a bundled extension?

Anyways, we could use some input so we can precede with confidence as we scale.

Cc: @CodeEditApp/maintainers

[nanashili]

I think you are very confused on what should be made an extension and what shouldn't. Anything inspector, navigator or even the text view should be core elements and not extensions not even built in extensions. It should be core components at all times.

[matthijseikelenboom]

Basically through the use of Protocols and Generics

I suspected as much... And I agree, that's not a realistic task right now.

Correct me if I am wrong, but won't we need dependency injection anyways if we want to be able to install and activate extensions without having to restart the app?

Can't confirm if it's entirely the same, but yeah, we "inject" a instance on the ExtensionAPI class in to the extension. But we also instantiate the class that extends the ExtensionBuilder, so I'm not sure if it counts as dependency injection 😅

[lukepistrol]

@austincondiff Yeah kind of. When we talk about extensions, they will need to conform to a protocol and implement the protocols methods and properties. When talking about dependency injection the main goal is, to have a protocol which defines a bunch of properties and methods, that the "injected" object will have to have implemented. This enables us to make it "Type"-independent. So we do not require a specific type of a class as an argument - it will instead be a generic type that conforms to said protocol.

This would look something like that:

protocol CustomProtocol {
  func doSomething()
  var someProperty: String { get set }
}

class SomeModel: CustomProtocol {
  func doSomething() {
    // needs to be implemented
  }

  var someProperty: String = "needs to be implemented too"
}

// then in a module:

init<T: CustomProtocol>(someModel: T) { 
  // we don't care if the object is of type `SomeModel`
  // it can be anything that implements `CustomProtocol`

  print(someModel.someProperty) // we can now do stuff with `someModel`
}

[MarcoCarnevali]

Yeah modules are nice for a couple of reasons. First of all it makes unit tests easy since we are able to test the single module / single unit in an isolate environment. It also forces you to write better code since the same module should be reusable and should work in isolation.
This really helps in the long run as the code is more modular and easy to change/update.
Protocol as Luke said are one of the solution for dependency injection (which helps make tests possible). The client pattern is also a possibility and I think it's even much simpler to use and maintain. An example of client is the git client and shell client we had

[matthijseikelenboom]

It also forces you to write better code since the same module should be reusable and should work in isolation.

Well, this we failed at, or at least enforcing it in PR's.

The client pattern is also a possibility and I think it's even much simpler to use and maintain

What do you mean exactly by this? That the modules should only contain the logic and no views? Because in the example your mentioning, Git depends on ShellClient right now, and it seems that is because of the views.

[matthijseikelenboom]

@MarcoCarnevali @lukepistrol @austincondiff So there needs be made a decision here. i don't feel qualified at all, if I look at my experience in Swift and Xcode and stuff, to make that decision.

What i do see as another possible option, is remove all the UI and Views from the modules and put them in the main app. It seems that the dependencies are (mostly) required when creating the Views. With the exception of AppPreferences.

[matthijseikelenboom]

To elaborate on the case of ToolbarBranchPicker, the rest of the related code of the this class is already in the main app. I thought about the possibility to move all the toolbar stuff to a new module, but the toolbar seems to be a part of the Window. So perhaps we should define what a part of the design is main app?

[austincondiff]

It’s not a bad idea. Maybe we should include parts of the main window in the main app. Here’s what this might look like.

We’d move all of the toolbar, navigator sidebar (not each navigator within the navigator sidebar), inspector sidebar (not each inspector within the inspector sidebar), debugger area drawer (not each tab in the debugger area), tab bar, and breadcrumbs into the main app.

Then we’d disperse all of the navigators, inspectors, and debuggers from the navigator inspector and debugger modules into their relevant modules. For example, the source control navigator and inspector would be moved into the SourceControl (now Git) module.

[matthijseikelenboom]

Update on this issue, after the meeting on 11/05/2022

We came to the conclusion that we are going to try to split the logic and views in the app. The reason for this is that some modules depend upon another because they require its views.

The logic, if possible, will stay in separate modules so they can be reused. Also this way the logic can be tested separately from the rest.

The views will move to core application.

In case of ViewModels, we'll have to check whether it makes sense to move them in the core as well or if it's better that it stays in it's respective module.