DeclarativeHub / ReactiveKit

A Swift Reactive Programming Kit
MIT License
1.24k stars 115 forks source link

Swift 3.0 support #83

Closed iandundas closed 7 years ago

iandundas commented 8 years ago

Opening an issue to track Swift 3 support (r.e. Swift-3 branch).

srdanrasic commented 8 years ago

Now that Foundation is dropping NS prefix we are in conflict with its Stream type. This looks like a major issue. We'll probably need another renaming.

iandundas commented 8 years ago

I’m constantly (accidentally) writing Signal instead of Stream (a remnant from my RAC days). A possibility?

(Though it’s inconvenient when popular libs collide, Property currently clashes with Realm’s Property type. So maybe not Signal.)

prefect42 commented 8 years ago

Whats the status of the the Swift 3 Branch?

srdanrasic commented 8 years ago

@prefect42, you can expect first Swift 3 beta in one or two weeks.

There will be some breaking changes because Stream and Operation are now types in Foundation framework so we are in conflict. They will probably be renamed to SafeSignal and Signal respectively (thanks @iandundas for suggestion). I don't imagine anyone using two reactive frameworks at once.

iandundas commented 8 years ago

Do you mean this:

Stream -> SafeSignal Operation -> Signal

I don’t get the rationale for the new names (particularly Operation->Signal). Seems it’s not as descriptive as to what it does?

Also if we’re renaming things, FYI ReactiveKit.Property conflicts with Realm.Property - probably a lot of people will come across this clash (namespacing of course fixes it but it’s a bit of a hassle)

On 27 Jul 2016, at 09:43, Srđan Rašić notifications@github.com wrote:

@prefect42, you can expect first Swift 3 beta in one or two weeks.

There will be some breaking changes because Stream and Operation are now types in Foundation framework so we are in conflict. They will probably be renamed to SafeSignal and Signal respectively. I don't imagine anyone using two reactive frameworks at once.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

frankschlegel commented 8 years ago

I also don't understand SafeSignal. What's safe there?

@iandundas You can always use type aliases in your project if you get conflicts like that. That shouldn't prevent ReactiveKit of choosing meaningful names.

iandundas commented 8 years ago

@frank Ah yes indeed - good point

srdanrasic commented 8 years ago

The underlying reason for those names is that Operation and Signal share 90% of the implementation - difference is that you can send errors only to Operations.

Operation would become Signal<Element, Error: ErrorProtocol> and Stream would become SafeSignal<Element>. In other words, SafeSignal is a Signal that cannot error out.

Additionally, as Swift now supports generic typealiases we can use the implementation of Operation/Signal for SafeSignal: typealias SafeSignal<Element> = Signal<Element, NoError>. With that we can reduce amount of code in the framework by 30% while keeping same functionality.

This was the direction I was leaning to, but I'm open to your suggestions.

iandundas commented 8 years ago

Hmm - what I like about the name “Operation” is that it gives the sense that it’s doing work which completes or fails, whereas a Stream gives the feeling that it’s not doing work and never ends. I don’t think SafeSignal/Signal give that sense any longer.

Would the second point fix the issue where flatMapping an Operation/Stream with another Operation does not allow .complete and .failure events to reach the observer?

On 27 Jul 2016, at 10:58, Srđan Rašić notifications@github.com wrote:

The underlying reason for those names is that Operation and Signal share 90% of the implementation - difference is that you can send errors only to Operations.

Operation would become Signal<Element, Error: ErrorProtocol> and Stream would become SafeSignal. In other words, SafeSignal is a Signal that cannot error out.

Additionally, as Swift now supports generic typealiases we can use the implementation of Operation/Signal for SafeSignal: typealias SafeSignal = Signal<Element, NoError>. With that we can reduce amount of code in the framework by 30% while keeping same functionality.

This was the direction I was leaning to, but I'm open to your suggestions.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

srdanrasic commented 8 years ago

I share your feelings around Operation name. However, I wasn't able to come up with anything that would preserve that impression. But maybe we should not think about it like that because both Operation and Stream do work and can end. Difference is just that Stream cannot complete with an error (receive .failure event) whereas Operation can. Thus, Stream is safe to, for example, be bound to a view.

I don't think this would fix the issue you had with not getting the completion events, if that's the one you are referring to. The reason is that that is not an issue, rather the correct behaviour. There should be other way around that.

frankschlegel commented 8 years ago

Task, Job, Process. Just throwing in some synonyms. ;)

srdanrasic commented 8 years ago

Task and Process are too part of Foundation / Standard Library :/ To be honest, I was never happy with the Operation either. It doesn't express the reactive aspect of the type.

mingyeow commented 8 years ago

my vote here is for a naming that makes RK as user friendly as possible, particular to new users.

I am not a fan of SafeSignal -> it is too verbose, given that is what the majority of my objects are. Also. for correctness, we willl have to name pushstream -> pushsafesignal, which just adds to the verboseness and difficulty to understand.

i would prefer keeping it as Signal / Operation. I like operation because it hints to the type of things you do with it. Or maybe Signal / FailableSignal if we really want to blend the 2 names.

Just my 2 cents!

iandundas commented 8 years ago

Same here, particularly:

it is too verbose, given that is what the majority of my objects are.

srdanrasic commented 8 years ago

Thanks guys for your feedback. Let's then rename Stream<Element> to Signal<Element> since this seems to be the most acceptable option.

We still have to reach an agreement about Operation. Problem is that NSOperation has been renamed to Operation so we can't use that name.

We can't use Task because NSTask is dropping the prefix. I'm also not sure that Task would be a good choice.

Although it could be a good fit, Process is already a type in Standard Library.

I'm not too happy with Job either because its main meaning refers to employment or paid work.

Here is few other ideas: Action, Attempt, Effort, Service.

Attempt is interesting because it expresses that failable aspect of operation. I could also like Service. What are your thoughts?

frankschlegel commented 8 years ago

What about Future and Promise? :: duck ::

srdanrasic commented 8 years ago

I'd reluctantly use already established terminology for something else, but this might be a good suggestion @frankschlegel.

What about having

Operation<Element, Error> == Attempt<Element, Error> Stream<Element> == Promise<Element>

and introducing concrete type for side-effects-free stream called Signal, something like

aPromise.shareReplay() => Signal<Element>

which would make Signal comply with the feeling that it’s not doing work as @iandundas imagined it and analogous to RAC's Signal.

mingyeow commented 8 years ago

I love attempt! It's a even better word then operation

How about this: Stream -> Signal (because it is very intuitive and should be kept simple) Operation -> Attempt

1) I'm not a big fan of having shareReplay return another type - I'm a big fan of not new terminology for less used terms 2) Promises never felt right, despite the wide JavaScript usage. Might also have pre-conception problems, since streams does not handle errors (unlike promises)

My 2 cents

iandundas commented 8 years ago

Cool yeah, Stream to Signal sounds good

Operation -> Attempt could be cool, though Attempt doesn’t get the feeling of having ‘next’ values, only attempt ‘completed’ or attempt ‘failed’. Or am I being too touchy-feeling here..

Not sure how Stream could be a Promise, though personally I’ve not used them so maybe I’m missing something (I went straight to ReactiveCocoa et al before I’d heard of them) but:

• Pending - the promise’s outcome hasn’t yet been determined, because the asynchronous operation that will produce its result hasn’t completed yet. • Fulfilled - the asynchronous operation has completed, and the promise has a value. • Rejected - the asynchronous operation failed, and the promise will never be fulfilled. In the rejected state, a promise has a reason that indicates why the operation failed.```



I thought Streams didn’t complete - and they also send `n>=0` next values..?

(p.s. I rather liked Stream 🚣🏻, shame that has to go..)
mingyeow commented 8 years ago

Yup, promises (as used in other services) has this state:

Rejected - the asynchronous operation failed, and the promise will never be fulfilled. In the rejected state, a promise has a reason that indicates why the operation failed.

That is why i am not a fan of using the nomenclature promises to replace rk stream (which are non-failable).

srdanrasic commented 8 years ago

I'm also fond of Attempt, although I agree with you @iandundas that it doesn’t get the feeling of having ‘next’ values. I doubt we'll ever find perfect name.

Thinking further, I agree with you guys on Promise. It's too well established name for a bit different concept. Let's not go into that direction.

Having side-effect-free type is quite important actually, I'd say. For example, say you have

class ViewModel {
  let status: Stream<String> = API.fetchCurrentUserStatus()
}

and then you do

viewModel.status.bindTo(labelX)
viewModel.status.bindTo(labelY)

What will happen is two network requests. We instead wanted status to share one request. If there was another type for such streams, I would use that type for status property so whoever is using my ViewModel has a guarantee that no redundant requests will be made. It's so easy to forget to call shareReplay()...

I would probably introduce another method instead of shareReplay for that purpose and it would be just an additional feature, not required to be used.

However, I'd like that type to be called Signal so that it has similar semantics as Signal in RAC, not to confuse people.

tonyarnold commented 8 years ago

Guys I don't think ReactiveKit should be changing it's lovely, simple classnames just because another library (including Foundation) has the same class name. This is what namespacing is for, we should use it.

srdanrasic commented 8 years ago

If it were any other library I would agree, but being in conflict with types from Foundation would make us write namespace prefix 90% of the time as Foundation is used almost everywhere. ReactiveKit.Stream is not that lovely :(

tonyarnold commented 8 years ago

Jerk Foundation being a jerk.

Righto, has work started on a branch for this? I need to get up and running in the next day or so, so I'm happy to throw some time/energy at the problem if you need me to.

iandundas commented 8 years ago

@tonyarnold I agree, I think

(Operation is a great name. But, if you do decide it needs to be changed, how about .. Undertaking instead? But yeah, just using namespacing could be best. ¯(ツ)/¯)

srdanrasic commented 8 years ago

@tonyarnold I have some work in progress. Let me wrap that up in a day or so and we'll see how to proceed.

iandundas commented 8 years ago

Stream2? 🛩

srdanrasic commented 8 years ago

We should go with Flux and rename Property to FluxCapacitor 💥

frankschlegel commented 8 years ago

Since we reached that point now (and since Swift is awesome), I suggest the following names:

💨⚡️🌪🚣🚀🛩🚿📣📢🔔💬

😋

mingyeow commented 8 years ago

@frankschlegel ah, you beat me to that idea! but seriously, we should have default typealias in there -> will be so much fun.

i see the point of having a 3rd type that will basically be sharereplay, and i like it a lot.

how about Signal, Operation and ConstantSignal?

This will keep signal/stream as the simple, main type, while making it clear that constantsignal's will not be triggered based on new observerations.

just 2 cents: 😁 😁 😁 😁 😁 😁

srdanrasic commented 8 years ago

Given it's so hard to find correct name, I'm seriously considering my first idea. Have just one reactive type by renaming Operation<Element, Error> to Signal<Element, Error> and instead of Stream just use typealias for non-failable signals.

typealias SafeSignal<Element> = Signal<Element, NoError>

This would be a solution for next year (until Swift 4) when I hope we'll finally have default generic parameters. Than we'll be able to define signal as Signal<Element, Error = NoError> and use it either like Signal<String> or like Signal<String, ReadingError>.

Alternative names could be

typealias Signal<Element> = Signal2<Element, NoError>

thanks to Ian's great idea that I though is a joke :D

Regarding side-effect-free signal type, ConstantSignal is good direction, but I'm not sure we're there yet.

frankschlegel commented 8 years ago

@srdanrasic All jokes aside, we trust you to pick good names. If we for some reason have issues with them, we can still define our own aliases. 😉

prefect42 commented 8 years ago

A signal implies that you will be signaled of something. Currently a stream and an operation are both "signaling" information. Ergo they should both be called Signals.

As to the error capability of an operation:
This is a signal which can also fail and was previously associated with work to be done. Therefore this is a signal as well as an operation (work)

This leads to my 4 cents: Stream, PushStream -> Signal, PushSignal Operation, PushOperation -> SignalOperation, PushSignalOperation

tonyarnold commented 8 years ago

Sorry to bump this guy, but I'm wondering why Type names like Signal1 and PublishSubject1 are being used. Is there a longer term plan to use more descriptive names for these types, or to combine them with their non-suffixed counterparts somehow?

srdanrasic commented 8 years ago

Signal1 is just

/// A convenience alias for non-failable signals.
public typealias Signal1<Element> = Signal<Element, NoError>

Yeah, it's bad name, but you can always use Signal<T, NoError> or even make your own typealias :)

Long-term, I hope with Swift 4 next year, we'll finally get default generic arguments. With that we'll be able to define signal as Signal<Element, Error = NoError> and then use it either as Signal<Int> or as Signal<Int, APIError>.

tonyarnold commented 8 years ago

Good point about using Signal, I'll do that for now. I wondered if you were waiting for something more elegant — the naming seemed inelegant compared to your usual approach, buddy 😉

prefect42 commented 7 years ago

Is there a compelling reason why the observe methods are declared as @escaping, instead of the Swift 3 default of @non-escaping?

This way you have to be careful not to forget to add weak/unowned capture references to the closure.

srdanrasic commented 7 years ago

@prefect42, yes, reactive programming is asynchronous in nature and the observer needs to live until the signal completes.

Trivial example is network request. When we register an observer, signal (request) is started, but it's not yet completed. Only at some later point in time will the observer receive data. And to keep the observer alive until that point, it needs to be escaped.

It's like dispatch async or UIView.animate's completion block. When we call it, we schedule an action. But the effect of that action will be received, i.e. observed, some time later. It's up us to specify if the objects that receive and process the results will be retained or not.

That's the price we have to pay when doing FRP. There are two things that can be done to alleviate that pain.

  1. More FRP code. Do as much logic as possible using just signals and their operators. Hopefully that we reduce the number of observations you have to make.
  2. Prefer bindings instead of observations. Check out Bond type from Bond framework to create your own binding targets. Theoretically you could replace all observations with these custom properties of Bond type.
prefect42 commented 7 years ago

Trivial example is network request. When we register an observer, signal (request) is started, but it's not yet completed. Only at some later point in time will the observer receive data. And to keep the observer alive until that point, it needs to be escaped.

That is exactly not intended behavior anymore! See: https://github.com/apple/swift-evolution/blob/master/proposals/0103-make-noescape-default.md

Most functional algorithms written in pure Swift will benefit because they are naturally noescape. The core team feels that this will reduce the boilerplate involved with writing these algorithms.

Lets take your example with the network request.

A user is tapping on a table view cell and is shown a detail page. The detail page is requesting some data from the server. The user then taps the back button and is no longer interested in the content shown.

The closure should not prevent my object from being removed. I do not want the responsibility of making my reference to the observer weak/ unowned. I can think of no scenario where it makes sense that a closure should make sure the observer is staying alive. You can always make sure of that yourself.

Lets look at another example and some code:

breathSlider.bnd_value.debounce(interval: 0.25, on: DispatchQueue.main).observeNext { [unowned self] breathValue in
    self.saveBreathLength(length: breathValue)
}.disposeIn(bnd_bag)

func saveBreathLength(length: Float) {
    // Function doing something with length
}

Here I have to make the reference to self unowned. Otherwise my view will never be deallocated and I have a retain cycle.

I would rather want to write much more functional code like this:

breathSlider.bnd_value.debounce(interval: 0.25, on: DispatchQueue.main)
    .observeNext(with: saveBreathLength)
    .disposeIn(bnd_bag)

func saveBreathLength(length: Float) {
    // Function doing something with length
}

But currently I can't, because this way the closure is retaining self strongly.

Sure. I could use more bindings, but I think the quoted Evolution proposal was made for FRP. @escaping here is counter productive!

Please think about my proposal of removing the @escaping in these and propably many more places! Thank you!

srdanrasic commented 7 years ago

Perhaps I have not expressed myself clearly. This is not an option, it has to be like that. You can't use a non-escaping closure asynchronously. I would not compile. The reason I create Bond type is exactly to get around this problem.

prefect42 commented 7 years ago

Duly noted! Thanks