touchlab / KMMBridgeKickStart

Apache License 2.0
33 stars 4 forks source link

Draft module structure for the SDK #20

Closed samhill303 closed 1 year ago

samhill303 commented 1 year ago

We want to demonstrate sub modules in the SDK. For starters, there should be an "umbrella" module on the outside, with a feature module inside of it

samhill303 commented 1 year ago

@russhwolf to draft a module architecture with the things we want to show

russhwolf commented 1 year ago

Here's what I think makes sense for repo/module breakdown (names not final)

SDK Repo

umbrella
├── analytics
├── breeds
    ├── database
    ├── http

umbrella only has iOS sources. Includes any iOS-specific API surface (eg callback wrappers around suspend funs) and exports analytics and breeds but not http and database. This is the module where KMMBridge is configured.

analytics is a simple module to make analytics calls. Maybe has a pluggable interface where apps can pass in their analytics provider, or we demo some default. The purpose of analytics is to demo a fairly minimal setup that might match a first-time PoC Kotlin module.

breeds is a top-level module for a thing that looks like KaMPKit's data layer (Mostly BreedRepository). It has platform-specific entry points so Android can pass a Context as needed. The purpose of breeds is to model a more complicated feature and highlight useful patterns. (We might consider having a separate breeds-api and breeds-impl so that umbrella can export only breeds-api, but let's not start there)

database is SqlDelight definitions and other DB-specific handling

http is Ktor client and configuration

I'm not sure how realistic the split between breeds, database, and http is for real projects, but it still functions as a demo of what it looks like to have layered modules.

Android Repo consumes breeds amd analytics as separate modules

iOS Repo consumes umbrella module

kpgalligan commented 1 year ago

I don't know if http needs to exist. Database serves to hide things from Sqldelight. HTTP feels like it's in a sub-module just to keep a similar organization structure to the database, but I don't think outside of visual "balance" that it really serves a purpose.

If Sqldelight didn't generate everything in a way that exposed it public, I don't think we'd have that sub-module breakdown (it would all be in "breeds").

I also think HTTP might want to talk to the db. Forcing them to be separate feels like something that's good in a conceptual sense, but will force some impractical design choices. Maybe. It'll definitely force DTO definitions in HTTP that are distinct from the ones generated in the db, which is not always how clients are designed.

Not the hill I'm planning to die on, but those are my thoughts.

Having analytics in shared Kotlin is so you can write typed calls (for example 'breedFavoriteClicked(id)' calls something like 'analytics.send("breedFavoriteClicked", Pair("id", id))' under the hood). I would expect both "breeds" to make calls to it, as well as the native clients. It allows you to centralize analytics calls so that neither platform needs to make sure calling basic strings doesn't "drift" and start doing different things. So, I wouldn't say it's just a minimal thing. Not that it should be big (neither module is really big).

Agree that android would consume each separately, and that the code in umbrella is iOS-only (because Android never sees it).

KatieGalvin commented 1 year ago

this needs more though and def on 10/14 @russhwolf

russhwolf commented 1 year ago

I can get behind dropping http if it overcomplicates things. I'm trying to think of not just SqlDelight classes as the things we might want to hide. Even if SqlDelight weren't always public, I'd want to have a layered module structure somewhere so we can demo deliberately not exporting something. But there are other strats for hiding other things (ie explicitly making things internal)

For DTOs I think we want HTTP and SqlDelight to be different (and we already have that in KaMPKit). I think communication between them can be coordinated in the breeds module (which is already how KaMPKit does things where the coordination happens in BreedRepository).

For analytics, you've thought about the details more than me but I think that structure sounds good. Less minimal that I was probably thinking initially, but the real point around minimalism is the gradle config is simpler than the other one.

kpgalligan commented 1 year ago

I've thought about the API guidance for a while. SQLDelight is a really special case because, left to it's own devices, it can expose quite a bit that you don't want or need. In most other cases, you just need good discipline with "internal". Putting a module in front of another module hides things, but then if you want things to poke through, you need the annoying "passthrough" redeclaration anti-pattern thing. In the (near) future, we should be able to suppress things like suspend functions we don't want, so the structural method of hiding things (with a module) becomes less useful. The only times you'll really get benefit from that vs "internal" is for cases like SQLDelight, where you can't control visibility. Over time, I've started referring to it as the "SQLDelight-case" as I think it'll gradually become a special case, and everything else is just good visibility control on your primary code.

kpgalligan commented 1 year ago

That isn't to say we shouldn't have modules, but creating modules solely for the purposes of controlling the Kotlin compiler's header/adapter generation for Objc is kind of it's own anti-pattern, and recent (or coming soon) updates dramatically reduce the cases where that's the best option.

samhill303 commented 1 year ago

https://github.com/touchlab/KMMSharedLibrary