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

Allow Binding<Case?> to write into nil or when the case matches #58

Closed
wants to merge 1 commit into from

Conversation

iampatbrown
Copy link

@iampatbrown iampatbrown commented Dec 20, 2022

Allowing Binding.case to always be writable may result in unwanted behaviour. If a binding changes case, it may be subsequently set to nil after the previous case is dismissed. Writing when the enum is nil or when the case matches could be a solution. I'm unsure if this is a common use case or if this change introduces some other unwanted behaviour. I just noticed a difference while replacing some customer helpers.

An example:

struct ContentView: View {
  @State var route: Route?

  enum Route { case foo, bar }

  var body: some View {
    VStack {
      Button("Show Foo") { self.route = .foo }
    }
    .confirmationDialog(
      title: { Text("Foo") },
      unwrapping: self.$route.case(/Route.foo),
      actions: { Button("Show Bar") { self.route = .bar } },
      message: { Text("Foo") }
    )
    .sheet(unwrapping: self.$route.case(/Route.bar)) { _ in
      Text("Bar")
    }
  }
}

@@ -59,6 +59,7 @@ extension Binding {
.init(
get: { self.wrappedValue.flatMap(casePath.extract(from:)) },
set: { newValue, transaction in
guard self.wrappedValue == nil || casePath ~= self.wrappedValue! else { return }
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open to suggestions on this. eg. use .map instead of == nil and force unwrapping

@tgrapperon
Copy link
Contributor

tgrapperon commented Dec 20, 2022

Hey @iampatbrown! This is a tricky problem (and I don't know the correct solution). The side-effect is that we'll lose legitimate/non-presentation uses of Binding<Case?>, albeit I can't formulate one using this helper directly right now. It is also an ad-hoc behavioral fix in Binding to a problem arising somewhere else, so I'm wondering if there aren't other solutions, like defining a Binding modifier that would be able to decorate an unconstrained case Binding with this kind of behavior, or more elaborate ones?

@iampatbrown
Copy link
Author

The side-effect is that we'll lose legitimate/non-presentation uses of Binding<Case?>, albeit I can't formulate one using this helper directly right now.

Yeah, I haven't really considered this in depth. It was prompted after reading #54 and realising I had been using a helper that was slightly different. I've just seen #57 as well though, which makes me realise I'm an episode behind and should have probably got up to date before making this PR. I'll leave it open for now. But feel free to close :)

@tgrapperon
Copy link
Contributor

tgrapperon commented Dec 20, 2022

As an alternative, one can imagine introducing a scope(…) variant, with the same signature as case(…) but where this behavior baked in. So we would use:

.sheet(unwrapping: self.$route.scope(/Route.foo)) {  }

making it clearer that this binding is "flat" and can only read/write Route.foo? out of $route. .case can assign any case, whereas .scope can only assign if it's matching the case or in a nil state.

I've also experimented with a general method that does the same on route:

.sheet(unwrapping: self.$route.track(/Route.foo).case(/Route.foo)) {  }

where .track(casePath) -> Binding<Enum?> is only effective on the casePath slice, but I don't think it makes sense in terms of API, and I guess a .scope() ~ .track().case() would be preferable.

@stephencelis
Copy link
Member

@iampatbrown Thanks for exploring this and sorry to leave it hanging for so long. While the change does make sense, I'm not sure the potential behavioral change is worth the risk, so I think we're going to close unless we can come up with a good reason to force the change on folks. We also have some case paths changes coming that will make many of these APIs deprecated (I'm interested to see if the new APIs suffer from the problem you saw, but I'm hopeful that they don't!), so stay tuned for that!

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

Successfully merging this pull request may close these issues.

None yet

3 participants