pointfreeco / swift-composable-architecture

A library for building applications in a consistent and understandable way, with composition, testing, and ergonomics in mind.
https://www.pointfree.co/collections/composable-architecture
MIT License
12.25k stars 1.42k forks source link

Making `Binding` conform to `Equatable` causes weird behavior in SwiftUI `Picker` view when using `@BindingState` #2041

Closed Jeehut closed 1 year ago

Jeehut commented 1 year ago

Description

After hours of debugging, I finally could fix a bug in my app which I explained in more detail here. So this bug actually doesn't affect me personally anymore due to my workaround, but I still want to report it to prevent someone else from running into it, and also to clarify if this is a SwiftUI bug or a TCA bug.

The bug was that when I made the Binding type conform to Equatable anywhere in my app like this:

extension Binding: Equatable where Value: Equatable {
   static func == (left: Binding<Value>, right: Binding<Value>) -> Bool {
      left.wrappedValue == right.wrappedValue
   }
}

This would affect all my Picker views that are using the @BindingState and viewStore.binding(\.$selection) when a value was set for the selection programmatically, e.g. on onAppear and cause the following weird behavior where you can see that the picker selects multiple values (as a comparison, I wrote the same Picker with vanilla SwiftUI where the issue doesn't happen):

Bildschirmaufnahme 2023-04-13 um 16 23 51

Here's the full code of the minimal SwiftUI Mac demo app I showed in the GIF above (I used the prerelease/1.0 branch, but I believe it happens on the stable releases, too):

import SwiftUI
import ComposableArchitecture

@main
struct AppView: App {
   let store = Store(initialState: AppFeature.State(), reducer: AppFeature())

   @State
   var selection: String?

   var body: some Scene {
      WindowGroup("Demo") {
         VStack {
            Picker("SwiftUI Selector", selection: self.$selection) {
               ForEach(["A", "B", "C", "D"], id: \.self) { selection in
                  Text(selection).tag(selection as String?)
               }
            }
            .onAppear {
               self.selection = "B"
            }

            WithViewStore(self.store, observe: { $0 }) { viewStore in
               Picker("TCA Selector", selection: viewStore.binding(\.$selection)) {
                  ForEach(["A", "B", "C", "D"], id: \.self) { selection in
                     Text(selection).tag(selection as String?)
                  }
               }
               .onAppear {
                  viewStore.send(.onAppear)
               }
            }
         }
         .padding()
      }
   }
}

struct AppFeature: Reducer {
   struct State: Equatable {
      @BindingState
      var selection: String?
   }

   enum Action: Equatable, BindableAction {
      case onAppear
      case binding(BindingAction<State>)
   }

   var body: some ReducerOf<Self> {
      BindingReducer()

      Reduce<State, Action> { state, action in
         switch action {
         case .onAppear:
            state.selection = "B"

         case .binding:
            break
         }

         return .none
      }
   }
}

extension Binding: Equatable where Value: Equatable {
   public static func == (left: Binding<Value>, right: Binding<Value>) -> Bool {
      left.wrappedValue == right.wrappedValue
   }
}

Checklist

Expected behavior

I expect the Picker to always select exactly one value.

Actual behavior

The Picker keeps the programmatically selected value selected with a checkmark forever.

Steps to reproduce

  1. Create a new Mac SwiftUI project
  2. Link TCA as a dependency (branch: prerelease/1.0)
  3. Copy the above code into the App.swift file
  4. Run the app

The Composable Architecture version information

prerelease/1.0

Destination operating system

macOS 13.3.1

Xcode version information

Xcode 14.3

Swift Compiler version information

swift-driver version: 1.75.2 Apple Swift version 5.8 (swiftlang-5.8.0.124.2 clang-1403.0.22.11.100)
Target: arm64-apple-macosx13.0
mbrandonw commented 1 year ago

Hi @Jeehut, it is not recommended to conform types that you do not own to protocols that you do not own, and there is even a proposal to warn about such conformances, and there's the potential it could even be made an error in Swift 6.

I'm almost certain that whatever strange behavior is being exhibited here would be reproducible in vanilla SwiftUI, but I wouldn't worry about it too much. At the end of the day, Binding should not be conformed to be Equatable by us.

I am going to close this because it is not an issue in TCA.

mbrandonw commented 1 year ago

I also want to highly caution against using Binding in your TCA state, even if you force it to be equatable with the wrapper. This is allowing reducers to communicate directly to systems outside of its state, and will eventually cause surprising, emergent behavior.

Jeehut commented 1 year ago

@mbrandonw I knew about the proposal (I actually summarized it myself here), but it also states in their example:

Now that this client has declared this conformance, if Foundation decides to add this conformance in a later revision, this client will fail to build.

So I thought if Binding would receive an official Equatable conformance, I would know it because my code would fail. But it doesn't fail. My guess is that there is some is Equatable condition in play somewhere in the library, which leads to the same conclusion: Don't extend existing types with existing protocols. Lesson learned. 😅

Jeehut commented 1 year ago

@mbrandonw Thank you for the input, to explain how I came to that solution: I'm using DocumentGroup in my app to let SwiftUI handle the opening of config files related to my app. It has an .init(viewing:viewer:) initializer where I get passed a FileDocumentConfiguration which I can write back to for editing purposed through a $document property which is a Binding. I didn't know better how to deal with that top-level situation that was fully controlled by SwiftUI back when I first implemented it, and as I wrote in the article, in my testing it all worked well.

Don't know if this particular usage is problematic and I'm also not sure how to do it differently. Any input that would direct me to a better solution would be appreciated! 👍

mbrandonw commented 1 year ago

@mbrandonw I knew about the proposal (I actually summarized it myself here), but it also states in their example:

Now that this client has declared this conformance, if Foundation decides to add this conformance in a later revision, this client will fail to build.

There's another reason to not retroactively conform besides compiler complications, which is that maybe the library designers specifically did not want the type to be Equatable. For Binding in particular, there is a lot more context behind a Binding besides its underlying wrapped value. For example, self.$model.name and self.$model.description are both Binding<String> and could both wrap the empty string "", yet should they be considered equal? They each came from two very different origins.

Jeehut commented 1 year ago

To explain my usage better, my body: Scene contains code that looks something like this:

DocumentGroup(viewing: ConfigurationDocument.self) { fileDocumentConfiguration in
   ConfigFileFeature.View(
      store: Store(
         initialState: ConfigFileFeature.State(
            configFileUrl: fileDocumentConfiguration.fileURL,
            configuration: fileDocumentConfiguration.$document
         ),
         reducer: ConfigFileFeature()
      )
   )
}

This allows me to change properties of the config file like this in child reducers the binding is passed down to:

state.configuration.projectName.wrappedValue = "Some String"

It even automatically saves the file, which is really a joy to use. But I'm not sure if this is the intended usage.

mbrandonw commented 1 year ago

Another option is to hold onto the state in your feature without the Binding, and then replay those changes to the binding in the view layer:

ConfigFileFeature.View(…)
  .onChange(viewStore.projectName) {
    fileDocumentConfiguration.document.projectName = $0
  }
Jeehut commented 1 year ago

@mbrandonw I've split the logic of editing the config file to over a dozen child features, and there are many different fields. If I understand your suggestion correctly, I'd need to .onChange on every possible field in all children and I might also easily forget to track one. I'm not sure if that's really a more viable solution. Bildschirmfoto 2023-04-13 um 18 14 25

This is allowing reducers to communicate directly to systems outside of its state, and will eventually cause surprising, emergent behavior.

But isn't the only "system outside its state" in my specific use case a mere "data passthrough" handled by SwiftUI? Yes, technically speaking I don't get informed about events such as that the file was saved. But I kind of like that aspect, SwiftUI just handles that part for me, I like to be not in control as it works well (so far).

It's good to know an alternative though for the case that it starts not working anymore in a potential future update of SwiftUI. I might rework the Binging solution then. Or am I missing something?