Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Question about reacting to state change #8

Closed
NeverwinterMoon opened this issue Jan 16, 2020 · 16 comments
Closed

Question about reacting to state change #8

NeverwinterMoon opened this issue Jan 16, 2020 · 16 comments

Comments

@NeverwinterMoon
Copy link

Hi!

First of all, a very awesome project and article on clean architecture with SwiftUI.

I have a question though.

You say Instead, they forward the result to the AppState or to a Binding. The latter is used when the result of work (the data) is used locally by one View and does not belong to the AppState.

For the first case, I can subscribe to any data change in Store by using something like routingUpdate and then .onReceive(routingUpdate) { _ in }. But I am a bit struggling with the latter case - I pass a binding to an Interactor when the data is only applicable to one view. I modify the binding in that Interactor, my views react to the change, but I also want to subscribe that change in the view and do some additional actions/interactions. How would I achieve that?

@nalexn
Copy link
Owner

nalexn commented Jan 16, 2020

Hey @NeverwinterMoon

You're not alone who struggled with this exact problem. Check out my comment here for the solution.

@NeverwinterMoon
Copy link
Author

NeverwinterMoon commented Jan 16, 2020

Thanks for a reply. I shouldn't have rushed in with my questions without checking the closed issues first :)

In fact, I have already tried using something like this in my current code base before trying out your approach to the architecture and thought that there might be something more elegant.

If you don't mind, I would like to ask something else using this same thread.

Before trying the architecture you've so nicely described and demoed, I was using enums (some with associated values) to represent navigation state and show different views/screens based on that. I also used view models and the root view's view model was watching logged in state and bluetooth permission status to be able to swap views whenever the user logs out or switched off bluetooth - which could happen pretty much on any of the screens I had. Now that I am trying to switch to the architecture proposed by you, I have difficulty organising such navigation.

I have something like in Root view:

var isLoggedInUpdate: AnyPublisher<Bool, Never> {
  injected.appState.updates(for: \.userData.isLoggedIn)
}

var bluetoothStatusUpdate: AnyPublisher<BluetoothStatus, Never> {
  injected.appState.updates(for: \.userData.bluetoothStatus)
}

var routingStateUpdate: AnyPublisher<Routing, Never> {
 injected.appState.updates(for: \.routing.root)
}

extension Root {
  struct Routing: Equatable {
    var scene: RootScene = .none // I am setting this depending one some conditions (user actions or data checks) to swap a view
  }
}

And I was trying to watch this data on root view in onReceive but surely it doesn't work out well enough. Every time root view's routing state is updated, onReceive gets the same values for isLoggedIn and bluetoothStatus and it breaks navigation flow. I want to be able to preserve the navigation flow created by user actions but so the automated navigation actions (watching login and bluetooth states) would only be triggered when these values actually change.

The idea is to do this: check if the user is already logged in, is not, show the login page, if the user is logged in, check if the bluetooth permissions were given already, if not, show a custom bluetooth screen, if the permissions were given, navigate to bluetooth device discovery screen.

It feels like I am doing something wrong and/or misunderstand something and it's driving me nuts. I am still very new to SwiftUI and the whole navigation approach here (compared to using Coordinator before) is baffling.

@nalexn
Copy link
Owner

nalexn commented Jan 17, 2020

The idea is to do this: check if the user is already logged in, is not, show the login page, if the user is logged in, check if the bluetooth permissions were given already, if not, show a custom bluetooth screen, if the permissions were given, navigate to bluetooth device discovery screen.

So you'd have something like this:

@State var isLoggedIn: Bool = false
@State var isBluetoothPermissionGranted: Bool = false

var body: some View {
    Group {
        if isLoggedIn {
            LoginView()
        } else {
            Group {
                if isBluetoothPermissionGranted {
                    DeviceDiscoveryView()
                } else {
                    BluetoothPermissionRequestView()
                }
            }
        }
    }
    .onReceive(isLoggedInUpdate) { self.isLoggedIn = $0 }
    .onReceive(bluetoothStatusUpdate) { self.isBluetoothPermissionGranted = $0.isGranted }
}

So your app's state already provides sufficient information for the screen to navigate to the correct destination screen. You don't need Routing struct for now.

@NeverwinterMoon
Copy link
Author

Thanks for the reply. This solution is clean and easy to manage, but in reality the code to check both login and bluetooth is asynchronous (Firebase Auth and centralManagerDidUpdateState), so I would like to wait for both of the states to return some result before processing them. In the suggested implementation, it's possible that the app would show a wrong view one moment and the next moment - the correct one. Say, isBluetoothPermissionGranted state is set a moment later than the isLoggedIn one.

What I tried today was using combineLatest on both streams, filtering it so both states receive a response, and then used bulkUpdate helper to update the AppState (but I was still using Routing struct for that update).

@nalexn
Copy link
Owner

nalexn commented Jan 17, 2020

You're trying to introduce the event-driven approach from UIKit, which is very foreign to the SwiftUI.

SwiftUI is powered by the state, not the events. You can mutate the state asynchronously, but you still need to define the state values that would reflect the "status is loading" for the app.

There is no need to "combine" the value streams. Let them be fully independent and update the corresponding values in the AppState.

All you need to do is to make sure "undefined status" for the bluetooth and isLoggedIn is representable by the AppState. This can be done with either making them Optional, or by wrapping in a Enum, such as Loadable

So, as your app launches and starts the async verification for the Bluetooth permission status, your ContentView should be able to see that the status is unknown yet, so it should show a loading indicator. Include this condition in the view definition.

SwiftUI is declarative. You need to declare how the UI looks like for specific set of state values. There is no events tied to the view directly - they only change the state, thus the view.

@nalexn
Copy link
Owner

nalexn commented Jan 17, 2020

The condition should be (in a presudo code):

if loginStatus.isUndefined || bluetoothStatus.isUndefined {
    LoadingView()
} else {
   // rest of the logic for the view's content
}

This way, these two async operations can complete in any order - the UI will show the loading indicator until BOTH complete.

@NeverwinterMoon
Copy link
Author

OK, I've tried out with just having state values. The more values I watch, the more often the view content update is triggered and the same view can be initialised multiple times. Meaning that if, say, loginStatus remains unchanged but bluetoothStatus changes and loginStatus.isUndefined || bluetoothStatus.isUndefined is still true, LoadingView() will be created again. Is this normal?

@nalexn
Copy link
Owner

nalexn commented Jan 19, 2020

Yes, this is how SwiftUI works. Views get re-created all the time by-design. You can skip the update by explicitly filtering equal values. I've written an article dedicated to this problem: https://nalexn.github.io/swiftui-observableobject/

I'd also recommend you to read another article of mine regarding the real performance bottlenecks in SwiftUI. The re-creation of the view hierarchy is SwiftUI is almost free, compared to UIKit. https://nalexn.github.io/anyview-vs-group/

@nalexn
Copy link
Owner

nalexn commented Jan 19, 2020

The loginStatus and the bluetoothStatus can be stored in AppState and updated independently, however, in your view you can introduce a single @State variable that would reflect a combined value of the two. This way you can mitigate the problem of multiple view redraws. You can use the approach similar to the one used in this sample project (where a CurrentValueSubject update is assigned to the @State), but filter the value after combining loginStatus with bluetoothStatus and before assigning to the @State

@NeverwinterMoon
Copy link
Author

I've read https://nalexn.github.io/swiftui-observableobject/. This article was the one that lead me to this project, by the way. I assume, you refer to this injected.appState.map { $0.viewState }.removeDuplicates().eraseToAnyPublisher() when you mention filtering the values. I am actually using almost the exact code from this demo project, so I have .onReceive(isLoggedInUpdate) which is backed by injected.appState.updates(for: \.userData.isLoggedIn) and updates is your helper:

func updates<Value>(
  for keyPath: KeyPath<Output, Value>
) -> AnyPublisher<Value, Failure> where Value: Equatable {
  map(keyPath).removeDuplicates().eraseToAnyPublisher()
}

Sadly, this filtering does not change the rate of view updates. Basically, every time any of the .onReceive(xxxUpdate) are triggered, the view is recreated, meaning that every var xxxUpdate: AnyPublisher<Loadable<SomeType>, Never> is delivered to onReceive with exactly the same value.

I can see it even in your demo project. The only thing I've changed was adding print statements and var blaUpdate: AnyPublisher<Loadable<Bool>, Never> which first sets loading, then loaded(true). Here is the output on initial launch:

Creating notRequestedView
routingUpdate: [Routing(countryDetails: nil)]
blaUpdate: [notRequested]
countriesUpdate: [notRequested]
keyboardHeightUpdate: [0.0]
Creating notRequestedView
routingUpdate: [Routing(countryDetails: nil)]
blaUpdate: [notRequested]
countriesUpdate: [notRequested]
keyboardHeightUpdate: [0.0]
notRequestedView:onAppear
countriesUpdate: [isLoading(last: nil)]
blaUpdate: [isLoading(last: nil)]
blaUpdate: [loaded(true)]
Creating notRequestedView
routingUpdate: [Routing(countryDetails: nil)]
blaUpdate: [loaded(true)]
countriesUpdate: [isLoading(last: nil)]
keyboardHeightUpdate: [0.0]
countriesUpdate: [loaded([...])]
Creating loadedViewView
routingUpdate: [Routing(countryDetails: nil)]
blaUpdate: [loaded(true)]
countriesUpdate: [loaded([...])]
keyboardHeightUpdate: [0.0]
Creating detailsView
Creating detailsView
Creating detailsView
Creating detailsView
Creating detailsView
Creating detailsView
Creating detailsView
Creating detailsView
Creating detailsView
Creating detailsView
Creating detailsView
Creating detailsView

Every time any of the state values changes, onReceive is triggered with the same value as before...

In my code the situation is even crazier for some reason. I sometimes get the same view recreated around 50 times without any state changes that would requires to swap views.

@nalexn
Copy link
Owner

nalexn commented Jan 19, 2020

Don't forget that SwiftUI is still not optimized so it can re-create the hierarchy a couple of times with no obvious reason (there could be the case it needs to re-calculate the layout, which would lead to this).

At least, I can tell that multiple records Creating detailsView is not a re-creation of the same view multiple times, but rather the creation of the destination views for all visible rows in the List.

If you have ForEach - this would make all the contained views being instantiated, which could be the reason for those 50 log messages you are observing.

@nalexn
Copy link
Owner

nalexn commented Jan 19, 2020

And regarding the repeated updates, such as keyboardHeightUpdate: [0.0] - every time the view is actually re-created, SwiftUI takes all the .onReceive modifiers and triggers the callback with the last actual value for the purpose of filling in the state of the view. It's not a duplicated update that somehow gets passed the filtering, it's how the SwiftUI makes sure your view is in a correct state after it has been re-created.

So there are 4 keyboardHeightUpdate: [0.0] messages and without digging further I can explain 3 of them - the counties list gets through 3 display stages - notRequested, isLoading, loaded

@NeverwinterMoon
Copy link
Author

NeverwinterMoon commented Jan 20, 2020

OK, it wasn't even ForEach. Basically, the problem was caused by Firebase. I discovered that even when I was not using any of the Firebase Auth methods to detect login, but was just having FirebaseApp.configure() in AppDelegate, any single state change on the initial view, would spam view re-creation over and over until some internal Firebase stuff was fully initialised. This would cause from 50 to over 100 re-creations with just one time state change.

I was able to reproduce the problem on iPhone 7 with iOS 13.3 but not on iPhone 11 with iOS 13.3.

When I removed the FirebaseApp.configure, the problem went away.

In the end, I tried this and it also solved the problem:

.onAppear {
  // This is a hack for, say, iPhone 7, where delayed Firebase init keeps recreating the same view: if the state
  // changes once, the view will keep on being re-created until Firebase is fully initialised.
  DispatchQueue.main.asyncAfter(deadline: .now() + 0.5) {
    self.checkStates()
  }
}

checkStates would check login and bluetooth statuses and then update the view @State values for each independently.

@NeverwinterMoon
Copy link
Author

NeverwinterMoon commented Feb 14, 2020

OK, I ran into another issue which is related to re-creating the same views. Hope it's not a problem that I just write it here.

Say, I have RootView that can present SomeOtherView based on state. SomeOtherView has an ability to change global state which RootView is also subscribed to. Whenever SomeOtherView changes that state, RootView actually re-creates SomeOtherView. Now, it wouldn't have been a problem, except SomeOtherView has a CancelBag().

So, the flow is: SomeOtherView triggers an action, that action updates some global property state to loading, RootView reacts to that change and re-creates SomeOtherView, meaning that the CancelBag is deallocated, so the action gets immediately cancelled and that's that...

Something like this:

struct RootView: View {
  private let cancelBag = CancelBag()
  @Environment(\.injected) private var injected: DIContainer

  @State private var routingState: Routing = .init()
  @State private var data: Loadable<()> = .notRequested

  var body: some View {
    Group {
      if routingState.showSomeOtherView {
        SomeOtherView()
      } else {
        Text("BLA").onTapGesture {
          // set showSomeOtherView = true
        }
      }
      .onReceive(dataUpdate) { self.data = $0 }
    }
  }
}

extension RootView {
  struct Routing: Equatable {
    var showSomeOtherView: Bool = false
  }
}

private extension RootView {
  var dataUpdate: AnyPublisher<Loadable<()>, Never> {
    injected.appState.updates(for: \.userData.someData)
  }
}

struct SomeOtherView: View {
  private let cancelBag = CancelBag()
  @Environment(\.injected) private var injected: DIContainer

  var body: some View {
    Text("Blah").onTapGesture { self.fetchData() }
  }
}

private extension SomeOtherView {
  func fetchData() {
    injected.actions
      .someInteractions
      .fetch() // this sets appState[\.userData.someData] = .isLoading(last: nil) and RootView re-creates SomeOtherView, so the action is cancelled
      .store(in: cancelBag)
  }
}

@nalexn
Copy link
Owner

nalexn commented Feb 14, 2020

@NeverwinterMoon I see the problem. You're right, this is the flaw in the design. I think the right approach would be to store the AnyCancellable token in the .isLoading enum value. This way it will have the correct lifetime and also provide the flexibility of canceling the operation at any moment.

The current approach was feasible in UIKit, as the lifetime of the view indicated the maintained interest of the user in the running operation, if they leave the screen - the operation could be canceled. But obviously SwiftUI needs a bit different treatment.

@nalexn
Copy link
Owner

nalexn commented Feb 15, 2020

I've updated the project to use the suggested approach. I think I should close this issue. Feel free to open a new one once you have another question!

@nalexn nalexn closed this as completed Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants