SpeziAccount & SpeziFirebase #107
Conversation
There was a problem hiding this comment.
I didn't manage to review all the changes today as I checked out the iOS implementation as well - (I know that the current state is a draft only). Some general notes so far, which we can then discuss next:
-
I notice a tendency to replicate exactly the iOS constructs. Here I would highlight two perspectives: 1. Developers of Spezi frameworks and 2. Users (developers) of Spezi Framework. Both actors are mainly used to develop natively in the respective platform, following the guidelines of the respective platforms - hence trying to replicate constructs of one platform into the other, might probably cause some confusion in regard to both maintaining the frameworks and also including and using the frameworks into their apps.
-
iOS is built keeping SwiftUI in mind and makes strongly use of
Environmentand property wrappers. While the equivalent construct in Compose isCompositionLocalI want to share some thoughts why it might not be the appropriate way to implement some of the features. Both components belong to the UI layer, and should ideally provide ui related data, such as theming for instance. By design, ui layer in Android is very fragile in terms of preserving state. The view / composables can get recreated multiple times during the lifecycle of the screen, e.g. during device configuration changes (dark / light mode for instance), hence it is not advisable to preserve any kind of state in UI layer. The appropriate handling of in Android should happen in ViewModels - those components preserve state even in case of view / composable recreations. It is not a nice architectural pattern to choose, but a MUST, especially when dealing with complex screens. Views should be treated and kept as dumb as possible, meaning that they should ideally just receive a reactive ui state injected and render themselves based on the state (e.g. compose state). Their job is also to notify user interactions (such as click actions). You can notice this in iOS as well from the strong usage of@MainActor. The main logic should then happen in the view model, which can process those and emit a new state again. I see this approach also applicable in SpeziAccount in kotlin. -
Before going into implementation, I would suggest us to start with the public API of SpeziAccount in iOS - which is what the users of Spezi will then consume in Android, a good start for that can simply be the documentation. I would then continue with identifying what is really needed to be public, e.g. service, configurations, models, views / composables. Next, we should identify what SpeziAccount needs to be provided from the users (e.g. configurations or extensions). Then we should proceed with the actual building of components. For composables, I see the concept of Slots API in compose which enables customization of the views that we will provide. In general, it is only the public components that we should align to provide similarly as in iOS (naming, configuration options, similar constructs), for internal constructs we shouldn't restrict ourselves to replicate iOS concepts, e.g using
Self.Typefor map keys - these are implementation details to achieve what the requirements of the public API
|
|
||
| private val standard: Standard = TODO() | ||
| private val storage: ExternalAccountStorage = TODO() | ||
| private val collectors = mutableMapOf<UUID, FlowCollector<Event>>() |
There was a problem hiding this comment.
FYI: we can use a concurrent hash map and can avoid the need for the mutex here
private val collectors = ConcurrentHashMap<UUID, FlowCollector<Event>>()| private val collectors = mutableMapOf<UUID, FlowCollector<Event>>() | ||
| private val mutex = Mutex() | ||
|
|
||
| val events: Flow<Event> get() = newSubscription() |
There was a problem hiding this comment.
Why do we need to keep a reference of the collectors in the map above? Currently we are manually setting entries there in newSubscription() and from the API I don't see a need to assign collectors from outside. This is handled by default from kotlin flow and coroutines api when consumers will start collecting the public events property . You can create a single private mutable flow here and expose via this public events. Also there is no need to handle onCompletion as you are doing below. Consumers of these events will launch their own coroutine scope to listen for changes, e.g. a viewModelScope. When the view model will get destroyed, the scope will get cancelled automatically -> and also will stop listening to these changes. By design flow apis should honor consumers request to listen to the changes, and we should not manage this internally via the map. Responsibility of this component should be to notify the events, and not manage it's listeners.
TLDR: I know this ways done to match the internal implementation in iOS, but the equivalent in android can be achieved as follows:
private val _events = MutableSharedFlow<Event>()
val events: Flow<Event> = _events.asSharedFlow()
// emitting e.g. in respond to event
_events.emit(event) // this will trigger a new update (emission) in public events flow
This approach is also safer in regard to wrong usages from clients, because they might access the events property multiple times from their components, which would result into creating new instances every time - while with the proposal we are returning the same instance (single source of truth) on every access
The only difference is that the shared flow is hot - technical meaning is that it will emit always even though it might not have any active subscribers / collectors. However, flow api is pretty efficient and we can really ignore the fact the we might emit even though no one is listening
| import kotlinx.coroutines.sync.withLock | ||
| import java.util.UUID | ||
|
|
||
| class AccountNotifications { |
There was a problem hiding this comment.
It would be great to align on the following pattern: Every public component will be defined as an interface, e.g. interface AccountNotifications, and the actual implementation as an internal class AccountNotificationsImpl : AccountNotifications. This approach will give the consumers of the library to use test doubles of their choice for testing, e.g. mocking or fakes.
Also, I would rethink the public API of this component, it should indeed not offer the respond to event method publicly as it allows mutation from outside, while we only want to mutate internally in Account class
| data class DisassociatingAccount(val details: AccountDetails): Event | ||
| } | ||
|
|
||
| private val standard: Standard = TODO() |
There was a problem hiding this comment.
we will define our own type right? as this is coming from android.text.style.TabStopSpan.Standard
There was a problem hiding this comment.
Yes, this is just a placeholder for now
SpeziAccount & SpeziFirebase
♻️ Current situation & Problem
This PR will add SpeziAccount functionality to Spezi. As long as it is still a draft PR, we may assume it not to be "finished" yet.
⚙️ Release Notes
Add a bullet point list summary of the feature and possible migration guides if this is a breaking change so this section can be added to the release notes.
Include code snippets that provide examples of the feature implemented or links to the documentation if it appends or changes the public interface.
📚 Documentation
Please ensure that you properly document any additions in conformance to Spezi Documentation Guide.
You can use this section to describe your solution, but we encourage contributors to document your reasoning and changes using in-line documentation.
✅ Testing
Please ensure that the PR meets the testing requirements set by CodeCov and that new functionality is appropriately tested.
This section describes important information about the tests and why some elements might not be testable.
📝 Code of Conduct & Contributing Guidelines
By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines: