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

CocoaAction Type Change #17

Closed
ashfurrow opened this issue Dec 17, 2015 · 14 comments
Closed

CocoaAction Type Change #17

ashfurrow opened this issue Dec 17, 2015 · 14 comments

Comments

@ashfurrow
Copy link
Member

It's <Void, Void> right now, but it would be awesome to somehow feed input into it. The problem is we can't define a property using generics on a non-generic type. I haven't given this a tonne of thought, so there might be an obvious solution.

@fpillet
Copy link
Member

fpillet commented Dec 17, 2015

That's one of the reasons why I mostly use Action<SomeType, Void> instead -- and that's perfectly fine! I view CocoaAction as just a shortcut for the most common case but don't have an issue with using Action when more flexibility is needed.

@justinswart
Copy link
Contributor

Submitted a pull request: #30

@ashfurrow
Copy link
Member Author

Could we use protocols to be sneaky here? Like:

typealias CocoaAction = Action<ActionInput, ActionOutput>

And then define those two protocols as empty and users can conform with their own types? Not sure. There's no easy answer, but I think it's an important discussion to have.

@justinswart
Copy link
Contributor

Hmm of course Swift 3 is going to come along in a little while and then we will have generic typealiases negating all of these workarounds. We could also consider a generic type wrapped in an enum/struct for now (the usual trick) until Swift 3? I'm not a huge fan of these but could work, eg:

struct GenericAction<Y, R> {
  typealias T = CocoaAction<Y, R>
}

let action = GenericAction<Void, Observable<String>>.T()

@ashfurrow
Copy link
Member Author

The problem comes from the rx_action property which, last time I tried, caused a compiler error in the UIButton extension.

@stefanomondino
Copy link
Member

It may sound stupid but.. why a CocoaAction has to be an Action?
In ReactiveCocoa they've implemented it as a standalone NSObject that can be assigned to UIControls (or its macOS equivalent, I'm not a Mac expert at all).
Basically it ties itself up to an existing Action and just provides a custom input parameter defined inside a closure whenever there's interaction with the control
I think it should be better to avoid useless creation of Actions (they involve too many observables in the process, it can be performance killer sometimes) if we only need a different input to perform the same thing.

@stefanomondino
Copy link
Member

stefanomondino commented Jan 3, 2017

I've tried to implement it (heavily "inspired" by RAC's CocoaAction) here
I've named it "ControlAction" so that it doesn't break current behaviour

The main drawback is that we're losing "Sender" type inside the closure, honestly I've never used it anyway.

@ashfurrow
Copy link
Member Author

Ah interesting, so it acts as like a wrapper for Action then? Neat idea. Anyone have feedback on this? Would love to discuss on a pull request.

@ishkawa
Copy link
Member

ishkawa commented Jan 3, 2017

I'm not sure that introducing Action wrapper is a right way for RxCocoa users.

I think our Action do not necessarily have to be the same as RAC's Action, because API design of each libraries differ from each other. RAC assumes that all actions triggered by Cocoa components is expressed as CocoaAction, but RxCocoa doesn't.

Additionally, we already have a mechanism that just provides a custom input parameter on Action.
inputs subject can receive flexible inputs like below:

let button = UIButton()
let barButtonItem = UIBarButtonItem()
let disposeBag = DisposeBag()

let action = Action<String, Void> { input in
    print("\(input) is pressed.")
}

button.rx.tap
    .map { _ in "UIButton" }
    .bindTo(action.input)
    .addDisposableTo(disposeBag)

barButtonItem.rx.tap
    .map { _ in "UIBarButtonItem" }
    .bindTo(action.input)
    .addDisposableTo(disposeBag)

Now, we have 2 choices to give a custom input parameters on Action:

  1. Use existing RxCocoa API and connect it to inputs subject.
  2. Add an action property to Cocoa components and set CocoaAction there.

Personally, I think 1 is more reasonable than 2.

@stefanomondino
Copy link
Member

@ishkawa this is really interesting, the main problem I see is that, in this way, the button's enabled property is not in sync with the action executing state.

I don't think either it's absolutely necessary to "mirror" RAC behavior, (otherwise I'd continue to use that instead of Rx.). It's not perfect at all, it's difficult to explain to new developers and has some "complications" that I don't need.
But, on the other hand, I don't see any downside to simply add a second "rx_action2" (I've called controlAction in my fork) that automatically many controls with a single action, with input customizations. It would not break (I think) any of the existing behaviors.

Let me also clarify why I came to this kind of conclusion.
Usually, when I do MVVM on iOS, I prefer to keep a single Action for each viewController's viewModel. In this way I can keep error and "showLoader" handling in one single place, and I don't have to replicate binding code for each action that could lead to an error (or could show a progress bar for long time operations). What really discriminates what the Action should do is the Input I provide to it. And the input depends on which button is pressed.
In this way, I have no business logic at all in the VC, i just have to tell the ViewModel to execute the Action with a converted input (usually an enum with all the possible choices in a viewModel). That's why I needed something different from your typealiased CocoaAction.

Maybe I can work out something on your suggestions that doesn't involve the creation of a new class, I'll let you know. In the meanwhile, THANKS :)

@stefanomondino
Copy link
Member

@ashfurrow should we close this? bindTo() should have taken care of this

@ashfurrow
Copy link
Member Author

Let's keep it open until we add documentation to the readme?

@stefanomondino
Copy link
Member

ok, I'm trying to do it right now BTW

@ashfurrow
Copy link
Member Author

Cool, thanks! And thanks again for adding it!

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

No branches or pull requests

5 participants