pointfreeco / isowords

Open source game built in SwiftUI and the Composable Architecture.
https://www.isowords.xyz
Other
2.67k stars 217 forks source link

Settings.Action.binding actions are not being handled as expected #170

Closed acosmicflamingo closed 1 year ago

acosmicflamingo commented 1 year ago

Describe the bug It seems like many of the viewStore actions sent within views of the Settings module (e.g. SoundsSettingsView) do not actually do anything within the Settings reducer as I'd expect. Instead, I see in the debug console a message like this:

2022-12-09 17:31:27.590658-0600 isowords[11473:1931144] [ComposableArchitecture] A binding action sent from a view store 
at "SettingsFeature/SoundsSettingsView.swift:22" was not handled. …

  Action:
    Settings.Action.binding(.set(_, 0.6017382))

To fix this, invoke "BindingReducer()" from your feature reducer's "body".

This is very strange because BindingReducer() is indeed within the Settings reducer body.

To reproduce If I compile isowords in Xcode, run the app, click on the settings button, select the sound button and try and change the value of any of the sound sliders, the sliders actually do not change value.

Expected behavior I'd expect that the Settings reducer would actually stop at any of the breakpoints I set within it.

Screenshots/video https://user-images.githubusercontent.com/67525430/206812842-45fda284-78ec-4908-8177-1ab66a1a5015.mov

Environment

Additional context What is strange is that if I run "SettingsPreview", I do not see this problem.

tgrapperon commented 1 year ago

Hey @acosmicflamingo! The other common error is forgetting to embed the child Reducer that hosts the BindingReducer into its parent. You can indeed check that the Home feature is not scoping any Setting reducer, which explains why it doesn't run at all.

If we add:

Scope(state: \.settings, action: /Action.settings) {
  Settings()
}

to Home.body, the message goes away and the sliders are updating.

The SettingPreview doesn't have any issue because it runs the Settings reducer by construction.

This is very likely an omission from the ReducerProtocol conversion, albeit I'm not familiar enough with the codebase to decide if this is the correct way to embed this feature here. I'll check how it looked like before, and I'll eventually PR.

acosmicflamingo commented 1 year ago

Hello there @tgrapperon ! Nice to meet you :) Thanks for the helpful info AND even creating a PR :D definitely makes sense. Since you say it's common, perhaps it'd be beneficial for this to be documented somewhere. So I created this PR: https://github.com/pointfreeco/swift-composable-architecture/pull/1729

stephencelis commented 1 year ago

This was fixed awhile back, and we finally converted our old style of shared settings state to use a proper dependency here: #186. Thanks for filing the issue though!