splendo / kaluga-swiftui

5 stars 0 forks source link

Updated stencil classes for kaluga 0.4.0 #21

Closed ChristoferAlexander closed 2 years ago

ChristoferAlexander commented 2 years ago

20

Updates Date and Color to KalugaDate and KalugaColor

ChristoferAlexander commented 2 years ago

@avdyushin I have some issues with the KalugaDate being an interface now. The Observable and UninitializedObservable generated classes along with their mapper classes have an input of type: typealias KotlinObject = AnyObject & Equatable

There are a couple of ways to tackle this.

@thoutbeckers tagging you in since you proposed the solutions

thoutbeckers commented 2 years ago

We could check in the Observable / UninitializedObservable if the input is instance of Equatable and do not do the comparisons in case it is not

@ChristoferAlexander Observables already do equality checks before emiting, so they should not be needed. So the question still is, where does the requirement of Equatable come from? (in other words, where in the code)

ChristoferAlexander commented 2 years ago

We could check in the Observable / UninitializedObservable if the input is instance of Equatable and do not do the comparisons in case it is not

@ChristoferAlexander Observables already do equality checks before emiting, so they should not be needed. So the question still is, where does the requirement of Equatable come from? (in other words, where in the code)

Following the whole chain:

struct SomeView: View {
    ...
    @ObservedObject private var stringText: StringObservable
    ...
    init(...) {
        let viewModel = SomeViewModel(...)
        viewModel = LifecycleViewModel(viewModel)
        stringText = StringObservable(viewModel.nameLabelText)
    }
}

Where:
StringObservable = typealias StringObservable = MappedWithDefaultObservable<NSString, String, StringMapper>

MappedWithDefaultObservable

As far as I understand some kind of mapping happens in the init of views from FlowInitializedObservable to the native ObservableObject

thoutbeckers commented 2 years ago

This is not an answer to why Equatable is needed. An easy way would be to remove Equatable from the definition and point to the code that breaks.

avdyushin commented 2 years ago

This is not an answer to why Equatable is needed. An easy way would be to remove Equatable from the definition and point to the code that breaks.

In first place Equatable was needed to ignore duplicate updates inside observable (by simple ignoring if new object the same as last received)

avdyushin commented 2 years ago

But this code was written long time ago, when Kaluga reported duplicated values:

https://github.com/splendo/kaluga-swiftui/blob/770204870ec82af924ac9036e02cbab53e677c1f/stencils/Observable.stencil#L56-L58

Maybe we can retest and if that's not a case (duplicates filtered inside kaluga) we can remove that

Usually we do distinctUntiChanged or use a StateFlow for UI observables so I do not think there should be an issue removing this. Worse case scenario we have to adjust some flows.

ChristoferAlexander commented 2 years ago

@avdyushin I updated the templates to not use equatable. Could you have a look?

avdyushin commented 2 years ago

Before merging this we have to create tag with current master like: kaluga-0.3.0

ChristoferAlexander commented 2 years ago

Before merging this we have to create tag with current master like: kaluga-0.3.0

Sorry @avdyushin, I completely forgot to merge this one. Not exactly sure how to tag it, could you tag it for me please?

thoutbeckers commented 2 years ago

@ChristoferAlexander https://github.com/splendo/kaluga-swiftui/tags or make a release there

ChristoferAlexander commented 2 years ago

@thoutbeckers, @avdyushin https://github.com/splendo/kaluga-swiftui/releases

Daeda88 commented 2 years ago

This actually doesnt compile right now. Making some changes to fix that

ChristoferAlexander commented 2 years ago

Thanks @Daeda88, noticed yesterday also when I was about to merge but was quite late and left it for today. I initially fixed the generated code locally and transferred the fixes to. the templates. Probably I missed those two.

ChristoferAlexander commented 2 years ago

Tested on xBike