Skip to content
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

Futures library #8

Open
craigzour opened this issue Jun 20, 2017 · 7 comments
Open

Futures library #8

craigzour opened this issue Jun 20, 2017 · 7 comments

Comments

@craigzour
Copy link

craigzour commented Jun 20, 2017

OMsignal would like to contribute to the community by bringing a small Futures library we have developed a few years ago.

https://github.com/OMsignal/ReactiveFuturesKit

@leonid-s-usov
Copy link

OMG thanks much!

@RuiAAPeres
Copy link
Member

@leonid-s-usov I am guessing by your reaction this would suit your needs?

@leonid-s-usov
Copy link

Well, it definitely looks like that! I'm reading the sources currently, but I can already see that's a great start for me.

@leonid-s-usov
Copy link

leonid-s-usov commented Aug 13, 2018

OK so after briefly reviewing the code I feel like this implementation has mixed the Future and Promise concepts (actually Promise is used only as a wrapper to the observer which will be fulfilled by the actual task)
Also, Future in this implementation is always cold and gets implicitly started by attaching one of onSuccess, onFailure or onCompleted handlers. This place is a bit problematic IMO, cause using onSuccess ignores all errors without any option to "catch" them by having this operator return another Future to attach to.

Not sure yet if that's doable in some less laborious way, but since Future class is not ProducerProtocol it doesn't inherit all operators which can be used for signals / producers, meaning a lot of code duplication. Maybe that's the only way to have it done, by copying the interface.

So, this is not Promises A+ and will not be, but it's still very nice, I'm happy that I was pointed to this implementation. But looks like this will only heat up a wish to spawn yet another layer library since I feel there's place for improvement :)

@RuiAAPeres
Copy link
Member

@leonid-s-usov let me know when and if you would like to spin this off, so I can do some prep work. 👍

@craigzour
Copy link
Author

@leonid-s-usov Hello :) We are glad you like it !

Our future implementation is not "cold" but "hot" because as soon as you initialize it by passing a SignalProducer we immediately subscribe to it and apply a replayLazily operator in order to use the value later on.

Here is the piece of code I am talking about:

public init(signalProducer: SignalProducer<Value, Error>) {
    self.futureProducer = signalProducer.take(first: 1).replayLazily(upTo: 1)
    self.disposable = self.futureProducer.start()
}

Concerning the fact that it is not ProducerProtocol compliant, I think it might be because we were lacking knowledge in Swift and ReactiveSwift. I do not know if it is really doable but would be definitely interesting.

@leonid-s-usov
Copy link

Hey, thanks for the response.

You are right, I have missed that start(). It would work without it as well, and would properly start the underlying producer on the first invocation of onSuccess, onFailure or onComplete, but would keep the semantics cold, which is what I have originally suspected.

In any case, I have gone for a trip of implementing a number of proof-of-concept APIs. You can check my proposals of Promise and AsyncValue / AsyncValueProducer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants