Cleanup public API

[mosamer]

trafficstars

Currently Action might be exposing more properties than needed. Optimally, it should look something like

                                    Action<Input, Element>
                      +----------------------------------------------+
                      |         enabledIf: Observable<Bool>,         |
       inputs ------->| workFactory: (Input) -> Observable<Elemenet> |------> elements 
(AnyObserver<Input>)  |                                              | (Observable<Element>)
                      +------------------------------+---------------+
                                                     |
                                                     |
                                                     |
                                                     +------> errors: Observable<Error>
                                                     |
                                                     +------> isExecuting: Observable<Bool>
                                                     |
                                                     +------> isEnabled: Observable<Bool>

Thus, following changes to be applied;

• enabled, should be renamed to isEnabled in conformance with Swift API design guidelines.
• executing, should be renamed to isExecuting in conformance with Swift API design guidelines
• _enabledIf to be declared as private
• workFactory to be declared as private
• inputs type to be changed to an ObserverType<Input>.

Some questions open for discussion;

• Should executionObservables remain public?
• Should errors be Observable<ActionError> or Observable<Swift.Error>? (may be related to #119 )
• Should inputs be AnyObserver<Input> or Binder<Input>?
• Do we still need InputSubject?

[ashfurrow]

Cool, this sounds like a good thing to discuss. I won't have to time consider until the weekend, but others should feel free to chime in 👍

[bobgodwinx]

• enabled to isEnabled makes sense, but we add enabled as deprecated to start with and later remove it totally
• executing to isExecuting same applies as above.
• _enabledIf and WorkFactory as private totally makes sense.
• inputs to ObservableType<Input> this could be tricky which means Inputs are no longer a SubjectType hmmm let's discuss this point in details. However if we go for ObservableType this means we could totally drop InputSubject
• inputs to be Binder<Input> sounds like a plan.

@ashfurrow we you agree we can mark this as approved ?

[ashfurrow]

I like all of these suggestions, especially the deprecation of the existing APIs instead of a replacement. Let's move forward with this, keeping in mind that we still welcome feedback from @RxSwiftCommunity/contributors on these changes 😃

[freak4pc]

+1 for mind-blowing ASCII chart :shipit:

[freak4pc]

BTW the InputSubject strategy was so you have non-terminating inputs. How do you solve this without it ?

[bobgodwinx]

@freak4pc valid point there. Maybe for now we leave that aspect and handle it later on. Because of the Non-terminating inputs I've thought of using ReplaySubject or BehaviorSubject but it doesn't make sense and even though you go with the share I am still not convinced because under the hood share are subjects. So for now I would suggest we keep the InputSubject

[beeth0ven]

Hi, there~ Is it the situation what PublishRelay design for:

PublishRelay is a wrapper for PublishSubject.

Unlike PublishSubject it can't terminate with error or completed.

Correct me if I'm wrong 👽

PR: Publisher - wrapper for PublishSubject that cannot error

[beeth0ven]

Sorry, I'm wrong 😔. This PR answer my question:

Binding of any observable that is finite or errors out is wrong IMHO, but right now we don't have compile time guarantees regarding those properties on Observable.

[mosamer]

@bobgodwinx @ashfurrow Deprecating renamed properties makes sense, but let’s keep in mind this cleanup is going to introduce breaking changes anyway. It’s also planned as part of a -sort of LTS- release so may be better to drop them now to avoid any future code churns. My suggestion here will be a Declaration Attribute that;

• mark them deprecated immediately
• mark them not available starting v 4.0 (or 4.1 if you think is better).
• provide a fix-it

As for inputs changes, the rational behind moving from SubjectType to ObserverType is an assumption that users should not need (or even be able) to do something like; myAction.inputs.subscribe(onNext: { /* do something with the input */ })

so this dropping of Observability also should eliminates PublishRelay as an option @beeth0ven

@freak4pc as for non-terminating strategy, IMO the simplest solution is an AnyObserver that simply ignore non-next events :) a Binder might be an alternative, but I think it might overcomplicate it unnecessarily.

Thanks all for your valuable feedback, I'd also appreciate your input on executionObservables and errors as well. If all is good I might be able to create a PR soon.

[freak4pc]

Thanks for the feedback @mosamer , Some great points here.

may be better to drop them now to avoid any future code churns

I agree with that. It's gonna be a major release probably as far as this codebase goes so I don't think we're risking making some deprecations as long as it doesn't require massive code-changes.

an assumption that users should not need (or even be able) to do something like

Agreed as well. We wouldn't want people to subscribe to the inputs, but we could just as well make InputSubject fatalError on a non-authorized subscription possibly (since it's our own implementation and not tied to RxSwift core).

I'm not sure if a AnyObserver would help solve the non-terminating issue, but it's also a good possibility.

[bobgodwinx]

ok guys let's agree on one thing here. We don't know how people are using Action in their daily work and removing InputSubject might cause some anger. So my advice is to keep it for now and it's working perfectly. I will mark it as approved and just like @ashfurrow said we can always review it

[freak4pc]

I know @fpillet is very busy but he's one of the more prominent users of Action (and wrote the chapter on it in the book IIRC), so if he'll have a minute to review this changes I would feel much better with just moving forward with this knowing a "hardcore" user of it feels comfortable making these changes for his use cases.

[mosamer]

I went throw Action's chapter on RxSwift cookbook and did not find anything contradicting these assumption. However, I'd prefer waiting for @fpillet input on the discussion whenever he finds time for it, no rushing.

[ashfurrow]

Good thinking @mosamer – I did the tech review for the book and I agree nothing about these changes would affect it. But that chapter is an introduction to Action, and I think the changes we're proposing are really things power-users are going to be using, and I want any disruption to their work to be minimized.

I would suggest we do the following:

• Aim to continue providing the existing API, but mark things that are being replaced as deprecated.
• Provide excellent in-line documentation around how to migration from deprecated.
  • Include a link that they can provide feedback on. Maybe there's a use case we're not considering and we should un-deprecate the API?
• Provide fixits.
• Keep the deprecated methods around a long time.

Are there any goals I'm missing? We could also move this to a Google Hangout or Skype call to discuss further.

[fpillet]

Hey guys, sorry to chime in so late. Been held back by insane workloads.

Okay so I looked at my usages on Action. As was mentioned, I'm a pretty heavy user, in all kinds of scenarios:

• I always expose Action from VMs
• typically set a UI element's action
• typically pass Action (of various forms) to cells for good decoupling of VM / VC / Cell
• sometimes use Action completely out of a UI context. Using it as a glorified closure
• I often bind to inputs to directly feed it with data
• I never watch what's going on through inputs
• I sometimes do subscribe to executionObservables and don't want them to be made private (I may use these to perform side effects not directly related to the action)
• If you remove InputSubject, how are we going to handle manual execution with execute ? I do use manual execution too, particularly in contexts where the action is triggered by another action, and I may or may not have data to go with that I could bind to inputs.

Now a couple things to consider. Action doesn't look very complicated but any change in its structure will impact its behavior. I remember fixing some particularly tricky issues in execute which was not always producing the expected result. The internal machinery is tricky to get right too, so my advice would be to be veeeeery careful when changing anything :)

[dangthaison91]

@mosamer Hi, I'm using Action with MVVM pretty much as @fpillet. One of the most important change will break my Pagination API is that inputs is AnyObsever so it cannot be observed it anymore. You can find usage in this PR #37 by @ishkawa.

[freak4pc]

Uhm this makes it a bit hairier to get rid of an Observable-style inputs.

The "conceptual" problem I have here is that an input isn't something you're supposed to be able to read at all (except for inside the class responsible for handling the inputs, in the case Action itself).

I don't have enough time to dive into the PR you mentioned but I'm sure there's a better way to deal with it possibly.

Any ways now that there seems to be a plausible use-case for having inputs as AnyObserver, I think we will have to keep it or very slowly deprecate it (with a proper replacement if there is one). Even though as I said earlier, I don't conceptually think subscribing to inputs make sense, seems like there are people counting on that side-effect.

[dangthaison91]

In some case, my Action failed and I want to get error paired with input value for display or revert local database/cache.

        invitationSettingAction
            .errors
            .withLatestFrom(invitationSettingAction.inputs) { $1 }.not()
            .subscribeNext { [unowned self] in self.updateInvitationSettingInRealm(status: $0) }
            .disposed(by: disposeBag)

Actually, we can have another Observable (inputObservable) then bind it to inputs so we can subscribe that Observable too. But It will have a problem if my inputsObservable is not paused emitting while executing.

Another approach is exposing an inputsObservable in PR #127 but I think it will conflict with this issues' subject is Cleanup API.

[mosamer]

@dangthaison91 Thanks for the insight, I will try to have a deeper look on the mentioned PR and the github repo for RxPagination before proceeding. I'm bit busy right now but hopefully will get back to you soon :)

[mosamer]

Hi @dangthaison91, after going through the mentioned PR #37 I can say migrating inputs to AnyObserver will still preserve the intended behaviour of triggering action by an arbitrary observable. As an ObserverType, you should still be able to bind to it.

Of course this migration will come with the following features broken;

• subscribe to inputs and get values passed by func execute(_:), which I believe is not a best practice.
• having a handy way to include inputs values in other business logic similar to your attached code or this, which can be easily refactored. Example:

let nextPageRequest = Observable
-   .combineLatest(action.executing, action.inputs, action.elements) { ($0, $1, $2) }
+   .combineLatest(action.executing, action.elements) { ($0, $1) }
   .sample(loadNextPageTrigger)
-   .flatMap { loading, lastRequest, lastResponse -> Observable<Request> in
+   .flatMap { loading, lastResponse -> Observable<Void> in
      if !loading && lastResponse.hasNextPage {
-         return Observable.of(baseRequest.requestWithPage(page: lastRequest.page + 1))
+         return Observable.just(())
      } else {
         return Observable.empty()
      }
   }
+   .scan(1) {current, _ in current + 1}
+   .map { baseRequest.requestWithPage(page: $0) }   

- let request = action.inputs
+ let request = Observable
   .of(refreshRequest, nextPageRequest)
   .merge()

+ request   
   .bind(to: action.inputs)
   .disposed(by: disposeBag)

IMHO, I believe these features were byproducts and may lead to anti-patterns and we should continue with this approach of replacing subject with observer.