Action icon indicating copy to clipboard operation
Action copied to clipboard

Why is the work only replaying the last value?

Open c0diq opened this issue 7 years ago • 6 comments

In Action.swift, only the last value emitted by work factory is being replayed. Is it intentional?

return Observable.of(workFactory(input)
                                             .do(onError: { errorsSubject.onNext(.underlyingError($0)) })
                                             .share(replay: 1, scope: .forever))

This have the side effect that trying to share the last execution, only the last value is being replayed.

c0diq avatar Oct 10 '18 05:10 c0diq

Yes; if we replayed all values, the memory use would grow forever as more executions are called. Rather than replay an arbitrary number of elements, we use 1 (which is necessary to compute internal state, if I recall correctly). Let me know what I can clarify.

ashfurrow avatar Oct 10 '18 20:10 ashfurrow

Understood. I wonder if it would be better to not replay anything at all in this case and calculate the state differently. Because one may incorrectly assume that only one value got sent.

c0diq avatar Oct 11 '18 17:10 c0diq

Yeah, I can see why you'd get that impression from reading the code. From the outside perspective, I think our API contract adheres to RxSwift norms. You wouldn't normally expect to get all values from a stream when you subscribe to it, but having the current value (that is to say, subscribing to the observable and doing stuff with it right away) is often useful. Let me know what I can clarify here 👍

ashfurrow avatar Oct 11 '18 19:10 ashfurrow

@ashfurrow @c0diq Should we remove implicit replay latest value and leave developer to decide if they need to?

dangthaison91 avatar Oct 22 '18 19:10 dangthaison91

I think we need the last value, if an existing Action gets assigned to .rx.action on seem UI element, that element needs to set its enabled state right away.

ashfurrow avatar Oct 22 '18 20:10 ashfurrow

@ashfurrow I see now we only need isEnabled to set enable states UI element .rx.action.

dangthaison91 avatar Oct 24 '18 16:10 dangthaison91