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

Concatening Signals in RAC 3 #1929

Closed
michaelmcguire opened this Issue Apr 21, 2015 · 12 comments

Comments

Projects
None yet
5 participants
@michaelmcguire
Copy link

michaelmcguire commented Apr 21, 2015

Is the lack of any sort of concatenation on the Signal type intentional in RAC 3.0? I'd like to take two signals and combine them to a new signal that emits all of the events of both. Seems like these sorts of operators are only available on SignalProducer.

Here is my use case: I've created an extension to UIButton that gives it a tappedSignal property of type Signal that emits an event with every tap. I would like to perform the same action when either of two buttons are tapped. At first, I thought I would just define a closure and pass it to observe on both button's signals but then thought I would try doing it via an operator (like the old concat:).

I'm not sure if somewhere along the line I A) have used the wrong type (my extension property on UIButton should be a SignalProvider instead of Signal), B) am approaching this the wrong way (just pass the shared closure to observe for both signals) or C) overlooking the correct operator on Signal. It sure seems like it is not A, as UI events seem to me to be the very essence of the new Signal type.

Thanks for any help!

@JaviSoto

This comment has been minimized.

Copy link
Member

JaviSoto commented Apr 21, 2015

Ran into this recently. There's no explicit operator for this (yet?), but you can easily do it with flatten :)

func merge<T, E: ErrorType>(signals: [SignalProducer<T, E>]) -> SignalProducer<T, E> {
    return SignalProducer<SignalProducer<T, E>, E>(values: signals)
    |> flatten(FlattenStrategy.Merge)
}
@michaelmcguire

This comment has been minimized.

Copy link

michaelmcguire commented Apr 21, 2015

Hey @JaviSoto, little confused by your function. It still operates on SignalProducer's (as does flatten). Worried I am missing something 😟

@JaviSoto

This comment has been minimized.

Copy link
Member

JaviSoto commented Apr 21, 2015

Oh, sorry, you asked about Signal, that's right. I think you can still use this method because there's a |> override that works with SignalProducer -> SignalProducer methods over `Signals? But I'm not sure right now and I can't check, but I would give that a try :)

@michaelmcguire

This comment has been minimized.

Copy link

michaelmcguire commented Apr 21, 2015

@JaviSoto it's no problem, I appreciate the help. There is a |> override that allows you to use Signal operators on SignalProvider's, but nothing that I can see that allows the reverse, which makes sense given the nature of the two classes.

@michaelmcguire

This comment has been minimized.

Copy link

michaelmcguire commented Apr 21, 2015

I created a (probably naive) implementation:

public func concat<T, E>(signal: Signal<T, E>)(otherSignal: Signal<T,E>) -> Signal<T, E> {
    return Signal<T,E> { observer in
        let firstDisposable = signal.observe(Signal.Observer { event in
            observer.put(event)
        })

        let secondDisposable = otherSignal.observe(Signal.Observer { event in
            observer.put(event)
        })

        return CompositeDisposable(ignoreNil([firstDisposable, secondDisposable]))
    }
}

I'm still learning how memory management and disposables work, and what I have doesn't seem quite right. It works in my case because these are infinite signals (button taps) but it seems like passing along the termination events without acting on them in any way here doesn't seem right.

@neilpa

This comment has been minimized.

Copy link
Member

neilpa commented Apr 22, 2015

The omission of join operators like concat on plain Signals is deliberate. It's too easy to inadvertently miss events when setting them up which can lead to unexpected behavior. There was a discussion about this somewhere but I can't seem to find it now.

@michaelmcguire

This comment has been minimized.

Copy link

michaelmcguire commented Apr 22, 2015

Thanks @neilpa, that makes sense for the reasoning. I'm wondering then if my mistake is the assumption that a UI element would provide a Signal? I can switch my extension to use a SignalProvider instead and get around this, but from what I've been reading it seemed a Signal was the more natural fit.

@neilpa

This comment has been minimized.

Copy link
Member

neilpa commented Apr 22, 2015

Yea, Signal is a natural fit UI element events.

Note, what you're calling concat is actually merge. A merge operator for Signal doesn't have the same potential problems as concat. However, adding one but not the other would require rethinking SignalProducer.join() since it unifies the two (as well as switch to latest).

That said, you should probably be using an Action and setting it as the target for both buttons.

@michaelmcguire

This comment has been minimized.

Copy link

michaelmcguire commented Apr 22, 2015

Great info, thanks.

As for Action, I am using those for handling the interaction between my view and view model. This specific use case was for the internal implementation of a custom UIView, and Action seemed like an awkward fit, but I will reconsider.

@jspahrsummers jspahrsummers added this to the 3.0 milestone May 3, 2015

@jspahrsummers

This comment has been minimized.

Copy link
Member

jspahrsummers commented May 3, 2015

I agree with @neilpa that merge is probably the only joining operator that avoids race conditions well enough, and I also agree that it might be hard to incorporate it into the API without conflicting with flatten and flatMap.

I'd tentatively say that we should leave it out of 3.0, and can always add it later if need be.

@harrywincup

This comment has been minimized.

Copy link

harrywincup commented Sep 10, 2015

@jspahrsummers @neilpa Is there a recommended way to achieve merge in RAC3 (or a better approach instead of needing to merge signals)? My situation is that I have multiple actions whose output is used to refresh the view. I don't need to know which signal sent the value and it shouldn't wait until all signals have sent a value (i.e combineLatest) ...

My method of solving this currently is to create a new Signal pipe(), use its observer as the argument for all of the signals I want to merge and then observe the new signal to get the stream with any of the values.

let (reloadSignal, reloadObserver) = Signal<[SubscriptionViewModel], NoError>.pipe()

fetchSignal         |> observe(reloadObserver)
unsubscribeSignal   |> observe(reloadObserver)

reloadSignal |> observe(next: { [unowned self] _ in
    self.collectionView?.reloadData()
})

It seems a bit verbose compared to

merge([fetchSignal, unsubscribeSignal]) |> observe(next: { [unowned self] _ in
    self.collectionView?.reloadData()
})

Am I way off?

@neilpa

This comment has been minimized.

Copy link
Member

neilpa commented Sep 10, 2015

Take a look at #1966. That includes an implementation of merge over signals-of-producers. We're also tracking bringing these back in #2315.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment