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

Revisit Action state-property convenience initialisers #455

Merged
merged 4 commits into from
Jun 15, 2017

Conversation

sharplet
Copy link
Contributor

@sharplet sharplet commented Jun 14, 2017

There are currently two options for using some property as state for an Action:

  • Use the designated initialiser Action.init(state:enabledIf:executing:) (the most complicated);
  • Use the various convenience initialisers defined for Void input types, which are nice but sacrifice the ability to use a custom input type.

This change rounds out the set of convenience initialisers for maximum flexibility:

  • You can now use a state-property with a non-Void input type, without having to worry about enabledIf;
  • You can use a conditionally-unwrapped optional state-property in the same way.

In order to unambiguously differentiate between the optional and non-optional variants in all cases, all the optional versions now use unwrapping as the parameter label for the state-property:

let model: MutableProperty<MyModel?>

let share = Action<UIViewController, Never, AnyError>(unwrapping: model) { model, viewController in
  return viewController.share(model)
}

// later...

button.reactive.pressed = CocoaAction(viewModel.share) { [unowned self] _ in self }

I'm pretty happy with the unwrapping label, as it feels much more self-documenting, and should hopefully make this series of convenience initialisers easier to navigate.

  • Updated CHANGELOG.md.

It is valid to conditionally unwrap an optional state property, but also
want to accept input of a different type. This change adds another
convenience initialiser to `Action` that conditionally unwraps, but also
preserves the `input` value passed to `apply()`.

The new overload uses the parameter label `unwrapping` in order to
distinguish it from the version of the initialiser that is generic over
`T`. Unfortunately, even if you use a property of optional values, the
compiler still selects the more-generic version that accepts any `T`
unless a different parameter label is used.
Whenever an action initialiser is intended to conditionally unwrap an
optional, it is now labelled as such to avoid ambiguity (both for
readers and for the compiler).
@mdiep
Copy link
Contributor

mdiep commented Jun 15, 2017

You can now use a state-property with a non-Void input type

Do you find yourself actually wanting to do this?

TBH I haven't used Action much (although I did use RACCommand). I'd envisioned that state would actually be the better model, since it's fully reactive. But I'm curious about how you're using it and would love to hear more.

@andersio
Copy link
Member

andersio commented Jun 15, 2017

In the codebase I'm working on, this is a recurring issue in our interactive view models (e.g. forms), requiring us to specify the closure input type explicitly to get the unwrapping overload.

@sharplet
Copy link
Contributor Author

You can now use a state-property with a non-Void input type

Do you find yourself actually wanting to do this?

The discussion on slack that these changes arose out of was specifically about using a UIViewController as the input to an action: I've experienced lots of cases where an asynchronous operation involves presenting some kind of modal UI, and it's a pretty common pattern in UIKit.

The main advantage of this change is the increased composability (wrapping/unwrapping, custom validation logic, input type) and progressive disclosure (start with a simple Void action, add state, then validation).

@mdiep
Copy link
Contributor

mdiep commented Jun 15, 2017

Sounds good. This just needs a changelog entry. 👍

@sharplet
Copy link
Contributor Author

This could probably use some documentation with examples at some point, but for now the changelog is updated 👍

@mdiep mdiep merged commit bdd2d26 into master Jun 15, 2017
@mdiep mdiep deleted the unwrapping-action branch June 15, 2017 20:18
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