Open mosamer opened 6 years ago
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 👍
enabled
to isEnabled
makes sense, but we add enabled
as deprecated to start with and later remove it totallyexecuting
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 ?
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 😃
+1 for mind-blowing ASCII chart :shipit:
BTW the InputSubject strategy was so you have non-terminating inputs. How do you solve this without it ?
@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
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
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.
@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;
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.
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.
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
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.
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.
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:
Are there any goals I'm missing? We could also move this to a Google Hangout or Skype call to discuss further.
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:
Action
from VMsAction
(of various forms) to cells for good decoupling of VM / VC / Cellinputs
to directly feed it with datainputs
executionObservables
and don't want them to be made private (I may use these to perform side effects not directly related to the action)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 :)
@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.
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.
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.
@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 :)
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
.flatMap { loading, lastResponse -> Observable
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.
Currently
Action
might be exposing more properties than needed. Optimally, it should look something likeThus, following changes to be applied;
enabled
, should be renamed toisEnabled
in conformance with Swift API design guidelines.executing
, should be renamed toisExecuting
in conformance with Swift API design guidelines_enabledIf
to be declared asprivate
workFactory
to be declared asprivate
inputs
type to be changed to anObserverType<Input>
.Some questions open for discussion;
executionObservables
remainpublic
?errors
beObservable<ActionError>
orObservable<Swift.Error>
? (may be related to #119 )inputs
beAnyObserver<Input>
orBinder<Input>
?InputSubject
?