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.08k stars 1.41k forks source link

TextField's value diverges from Binding when limiting characters (or any other restricting logic that keeps the end state the same) #380

Closed ldstreet closed 3 years ago

ldstreet commented 3 years ago

Description: When binding a TextField to a piece of Equatable state and layering on logic in the reducer that causes the the state to be unchanged (i.e. adding character limits), TextField diverges from its binding and renders the typed text even though it doesn't match the value held in state. This seems to be due to the view store filtering out the state change since the logic doesn't trigger an Equatable difference.

This might actually be a SwiftUI bug rather than a ComposableArchitecture bug since it seems to break the contract of a binding to have any other value but that bindings value render on screen.

Example:

import SwiftUI
import ComposableArchitecture

struct State: Equatable {
    var text1 = ""
    var text2 = ""
}

enum Action: Equatable {
    case textChanged1(String)
    case textChanged2(String)
}

let reducer = Reducer<State, Action, Void> { state, action, _ in
    switch action {
    case .textChanged1(let value):
        /// only keep first 5 characters
        /// because this logic does not cause state to change value, this logic will not be renedered after execution
        /// the view will still be updated with the typed text despite it not matching the bound value set here
        state.text1 = String(value.prefix(5))
        return .none
    case .textChanged2(let value):
        /// pick a random character to replace value with.
        /// because this logic causes state to change value, this logic will be renedered after execution
        /// because this state change is rendered, all typed text will be ignored
        state.text2 = String("abcdefghijklmnopqrstuvwxyz".randomElement() ?? "a")
        return .none
    }
}

struct TextFieldEquatableBug: View {

    private let store: Store<State, Action>

    init(store: Store<State, Action>) {
        self.store = store
    }

    var body: some View {
        WithViewStore(store) { viewStore in
            VStack {
                TextField(
                    "Bug Test 1",
                    text: viewStore.binding(get: \.text1, send: Action.textChanged1)
                )
                .textFieldStyle(RoundedBorderTextFieldStyle())
                .padding()

                TextField(
                    "Bug Test 2",
                    text: viewStore.binding(get: \.text2, send: Action.textChanged2)
                )
                .textFieldStyle(RoundedBorderTextFieldStyle())
                .padding()
            }
        }
    }
}

struct TextFieldEquatableBug_Preview: PreviewProvider {
    static var previews: some View {
        TextFieldEquatableBug(
            store: .init(
                initialState: .init(),
                reducer: reducer,
                environment: ()
            )
        )
    }
}

Expected Outcome: The TextField's text value should remain in sync with the value held in state.

Workaround: For now I can work around this by handling the removeDuplicates closure on WithViewStore explicitly rather than relying on the Equatable conformance...but this definitely seems suboptimal.

I could also add a piece of state that I modify simply to signal that the text has been processed via the reducer (toggle a boolean? set a UUID?) and thus re-render the UI. This is also less than ideal.

Environment:

stephencelis commented 3 years ago

Hi @ldstreet! @mbrandonw and I discussed this exact behavior a few weeks ago. While it's definitely a gotcha, we'd probably suggest the same workarounds right now if this is the behavior you want. We're definitely open to discuss improvements here, though!

Alternatively, you could potentially validate the text field in other ways if you're up for a different user experience:

lukeredpath commented 3 years ago

I think the simplest thing you could do to ensure the state changes when the user exceeds the truncation limit is to track the typed text separately from the truncated text in two separate properties. You could also use this to drive the appearance of a useful error message.

myurieff commented 3 years ago

What is working for me is to have two separate actions - one for the binding and one for the sanitisation:

struct ComponentEnvironment {
    var mainQueue: AnySchedulerOf<DispatchQueue>
    let sanitizeNameInput: (String) -> Effect<String, Never>
}

// .. 

ComponentEnvironment(
    mainQueue: DispatchQueue.main.eraseToAnyScheduler(),
    sanitizeNameInput: { Effect(value: String($0.prefix(5))) }
)

// .. 
let componentReducer = Reducer<Component, ComponentAction, ComponentEnvironment> { state, action, environment in
    switch action {
        case .nameDidChange(let name):
            state.ingredient.name = name
            return environment
                .sanitizeNameInput(name)
                .receive(on: environment.mainQueue)
                .map(ComponentAction.nameSanitizationResponse)
                .eraseToEffect()
        case .nameSanitizationResponse(let sanitized):
            state.ingredient.name = sanitized

        // ...
    }
}

✌️

stephencelis commented 3 years ago

I'm going to convert this into a discussion since we don't currently consider it a bug, but we are down to improve things with feedback.