How do we keep the closure clean with the observe?

[y-mimura]

I'm currently in the process of migrating to 1.7, but faced the challenge of the title.
I had been used a lot of store.publisher or store.scope.ifLet on UIViewController to observe state changes before 1.7. For instance:

override func viewDidLoad() {
  setupObservers()
}

private func setupObservers() {
  observeAppearance()
  observeLogin()
  // ...
}

private func observeAppearance() {
  store.publisher.appearance.sink {
    // ....
  }
}

private func observeLogin() {
  store.scope(
    state: \.login,
    action: \.login
  ).ifLet {
    // ...
  }
}

// ...

I'm replacing these API with observe, however, I would like to know how the code should be split up.

For instance, should one observe contain multiple functions?

override func viewDidLoad() {
  observe { [weak self] in
    guard let self else { return } 
    observeAppearance()
    observeLogin()
    // ...
  }
}

private func observeAppearance() {
  let appearance = store.appearance
  // ....
}

private func observeLogin() {
  if let loginStore = store.scope(state: \.login, action: \.login) {
    // ...
  }
}

// ...

Or should we use multiple observes?

override func viewDidLoad() {
  observeAppearance()
  observeLogin()
}

private func observeAppearance() {
  observe { [weak self] in
    guard let self else { return } 
    let appearance = store.appearance
    // ....
  }
}

private func observeLogin() {
  observe { [weak self] in
    guard let self else { return } 
    if let loginStore = store.scope(state: \.login, action: \.login) {
      // ...
    }
  }
}

// ...

[acosmicflamingo]

I had this question as well as I was migrating my UIKit codebase to using the new Observability framework. What Brandon had told me is that in general, you should be safe to add everything in a single observe closure and that executing something like myButton.title = store.name multiple times is safe. If you were to be doing something more computationally intensive like reloading a table view, then you would want to have separate observe closures. This discussion might help you here: #2722.

[y-mimura]

@acosmicflamingo
Thank you for the reply! Actually, after reading the discussion #2722, I thought I should split it up.
In my case, I had been used store.scope.ifLet to decide that should execute present or not. Therefore, I believe that observe should be separated in my case since present might be executed multiple times if the store.appearance (in my example) was changed.

[acosmicflamingo]

Glad I can help! That certainly sounds like what I'd do too ;) Good luck!

[mbrandonw]

Hi @y-mimura, while it is completely fine to split out into multiple observes, it is also possible to put it in one and do navigation. Our TicTacToe demo shows how to do this:

swift-composable-architecture/Examples/TicTacToe/tic-tac-toe/Sources/LoginUIKit/LoginViewController.swift

Lines 102 to 123 in 4c6b617

	if let store = store.scope(state: \.alert, action: \.alert),
	alertController == nil
	{
	alertController = UIAlertController(store: store)
	present(alertController!, animated: true, completion: nil)
	} else if alertController != nil {
	alertController?.dismiss(animated: true)
	alertController = nil
	}
	if let store = store.scope(state: \.twoFactor, action: \.twoFactor.presented),
	twoFactorViewController == nil
	{
	twoFactorViewController = TwoFactorViewController(store: store)
	navigationController?.pushViewController(
	twoFactorViewController!,
	animated: true
	)
	} else if store.alert == nil, twoFactorViewController != nil {
	twoFactorViewController?.dismiss(animated: true)
	twoFactorViewController = nil
	}

In one single observe block we are configuring various UI components, as well as presenting/dismissing an alert and drill-down. You just need to do some extra nil checking to make sure it is time to do the presenting or dismissing.

[y-mimura]

@mbrandonw Thank you for showing this demo. That was exactly what I'd like to know. However, one thing I have wondered about in this demo.

The conditions for displaying alerts are as follows:

swift-composable-architecture/Examples/TicTacToe/tic-tac-toe/Sources/LoginUIKit/LoginViewController.swift

Lines 102 to 103 in 4c6b617

	if let store = store.scope(state: \.alert, action: \.alert),
	alertController == nil

However, I think it should be divided as follows, is my opinion correct?

  if let store = store.scope(state: \.alert, action: \.alert) {
    if alertController == nil {
    // ...

In my understanding, the closure of the observe will be executed each time the referred store property was changed. Therefore, if the conditions are combined, when other than the alert state was changed during the alertController is presented, the alertController might be dismissed unexpectedly.

[mbrandonw]

Hi @y-mimura, you are right it's not quite correct. The two factor navigation just below is the correct way to do this kind of logic:

swift-composable-architecture/Examples/TicTacToe/tic-tac-toe/Sources/LoginUIKit/LoginViewController.swift

Lines 112 to 123 in 4c6b617

	if let store = store.scope(state: \.twoFactor, action: \.twoFactor.presented),
	twoFactorViewController == nil
	{
	twoFactorViewController = TwoFactorViewController(store: store)
	navigationController?.pushViewController(
	twoFactorViewController!,
	animated: true
	)
	} else if store.alert == nil, twoFactorViewController != nil {
	twoFactorViewController?.dismiss(animated: true)
	twoFactorViewController = nil
	}

The alert condition should be more like this:

if let store = store.scope(state: \.alert, action: \.alert),
  alertController == nil
{
  …
} else if store.alert == nil, alertController != nil {
  …
}

In particular, the else branch should check that store.alert is nil before dismissing. I'll PR a fix soon.

[y-mimura]

@mbrandonw Ah, it's crystal clear now. I appreciate your explanations 👍