copper-leaf / ballast

Opinionated Application State Management framework for Kotlin Multiplatform
https://copper-leaf.github.io/ballast/
BSD 3-Clause "New" or "Revised" License
144 stars 11 forks source link

Refactor Repository module #31

Open cjbrooks12 opened 1 year ago

cjbrooks12 commented 1 year ago

After some time using and thinking about the Repository module, I've come to the conclusion that it's current stated use-case is not the most ideal for structuring your application. Most notably, the method of using an EventBus for communication between Repository instances is flawed. Instead, Repositories that depend on the state of other Repositories should be arranged in a hierarchy (either a true tree, or a Directed Acyclic Graph (DAG)), rather than something like an unstructured Graph as is the current setup.

The inspiration for this change comes from thinking about the application as a whole, and understanding the purpose of the Repository layer and the Repositories within that. Just as the UI screens and their ViewModels only ever passively observe from Repositories, and thereby isolate themselves from the implementation details of the Repository while also eliminating strange communication patterns by enforcing the UDF flow, the same should be true of Repositories amongst themselves. Parent repositories should send their state to children, which eventually flows to UIs, to deeper components within the UI, etc.

Ultimately, it should follow a flow that is something like this:

flowchart TD
    Global["Global State (authentication)"]
    Router
    Repositories
    Screens
    Components

    Global --> Router
    Global --> Repositories
    Router --> Screens
    Repositories --> Screens
    Screens --> Components

The extent of changes should be as follows:

Additional nice-to-have features include:

rocketraman commented 1 year ago

Found this issue linked from Slack.

Thank you for this. I found when getting started with Ballast the repository concept to be among the most difficult to understand and create the "correct" architecture for. I believe these changes will go a long way to improving that.

When a repository is "injected" into another via constructor, would the "parent" repository simply call public methods of the injected repository? Or would there be some kind of intermediate layer provided by Ballast that helps manage and control that communication?

cjbrooks12 commented 1 year ago

The idea would be to only use the public methods of one repository in another, and passing one repo into another via constructors enforces a hierarchy among them. Mostly, it would be a rewrite of the documentation, with some additional utilities to help with specific features commonly used in repositories like caching API calls or observing flows from a DB.

rocketraman commented 1 year ago

The idea would be to only use the public methods of one repository in another, and passing one repo into another via constructors enforces a hierarchy among them.

This is what we found worked best on our project as well. Great!

rocketraman commented 1 year ago
  • Rewrite documentation to encourage passing parent ViewModels into the constructor of another

View models at the UI layers usually depend on some local context, most importantly some lifecycle information e.g. coroutine scope.

Therefore, presumably these lower layer view models won't be passed around in constructors.

Only view models that are instantiated at app startup and live for the entire time the application is running would be passed into constructors right? Will there be a simplified way to create view models such as these, such that they can be created in a DI system easily?

rocketraman commented 1 year ago

Also, when communicating state from parents to children in response to children calling their parents, it would be great to have a canonical way to solve the problem I mentioned to you in some Slack DMs a while back.

Using CompletableDeferred was one option we had discussed, and works but is ugly. I would have preferred passing lambdas that update the local state instead, which I think is a reasonable way for a parent VM to update the state in a child VM (similar to how a child UI component might update state in a parent). However, this approach does not work well due to a state exception when the lambda is executed:

IllegalStateException: This InputHandlerScope has already been closed

When thinking about refactoring the repository module, is it worth considering some built-in way for a parent repo to communicate state back to a child in response to a call on the parent?

cjbrooks12 commented 1 year ago

Therefore, presumably these lower layer view models won't be passed around in constructors.

That's correct, UI ViewModels should never depend on one another, they can only depend on the Repositories. It's only the "Repository ViewModels" (or just "Repositories") that depend on each other, and when they do, it's in a strict hierarchy enforced by constructor injection.

Will there be a simplified way to create view models such as these...

I haven't thought this through entirely yet, but the main thing I've got in my head (and what I'm doing in my projects right now) is to just use a normal BasicViewModel, where the CoroutineScope is injected internally from within DI rather than passed into a factory DI function.

communicating state from parents to children

This is the main thing I'm still thinking through, both for how to easily manage a lot of state values in the Repository, and how to easily share those with the children dependencies. I don't think it should stray far from the current model for consuming it (parent state will still just be exposed as a StateFlow), but there is definitely opportunity to simplify this whole process, and I've got a few ideas that might work.

And yes, this would need to cover all use-cases of:

1) The Repo observing a reactive data source and emitting a Flow 2) The Repo making a one-shot query internally and caching the result 3) A child requesting a one-shot change and the Repository returning a result

is it worth considering some built-in way for a parent repo to communicate state back to a child

It is absolutely worth considering, and I've got some ideas on how it could be implemented (for example, have the child State implement an interface that allows it to be updated directly from an Interceptor). I'll play around with the idea, but I'm not sure yet if it will be something that would be generalizable enough to include in the library, or just be a recommendation that you would handle yourself.