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

Stream of one event. #661

Closed
andersio opened this issue Jul 30, 2018 · 50 comments
Closed

Stream of one event. #661

andersio opened this issue Jul 30, 2018 · 50 comments
Assignees

Comments

@andersio
Copy link
Member

andersio commented Jul 30, 2018

aka Single #201 or (lazy?) Promise or (lazy?) Future.
Prototype

The RAC team is considering moving this forward. Apparently we would be able to provide a more concise API on top of the stronger compiler guarantees, and it is observed that the value-or-failure are quite commonly used, especially in modelling network requests or API queries.

A few design decisions need to be discussed:

  1. Should it be cold or "warm"?

    Proposal: Cold by default, opt-in replay via replayLazily()?

  2. Should it be called Promise or Future if it isn't "warm" by default?

  3. How should we model the event?

    Proposal:

    extension Promise {
        public enum Event {
             case completed(Value)
             case failed(Error)
             case interrupted
        }
    }
  4. Should we support operator lifting as part of the public API? It can be made safe if we always apply take(last: 1) as part of the lifting.

@mdiep
Copy link
Contributor

mdiep commented Jul 31, 2018

Some operators on SignalProducer could probably also return one of these instead of a SignalProducer. e.g. collect

@RuiAAPeres
Copy link
Member

This is a great idea, although I would like to see the same arguments used when I proposed this (#201) and got shot down. :) ( cc @NachoSoto and @liscio ).

@mluisbrown
Copy link
Contributor

mluisbrown commented Jul 31, 2018

I also think this is a great idea. Our codebase is littered with a convenience function which does:

map { transform($0).producer }.once()

I would say that (in our codebase) maybe 50% or more of SignalProducers are single value (eg, network requests). To counter what @liscio said against this idea in #201 I think it's clearer for both someone writing code or reading it to have a distinction at the call site between single value producers and others that produce a stream of values.

UPDATE: I also think that having Promise might make it easier for developers to transition from using Promise libraries like PromiseKit to ReactiveSwift.

@leonid-s-usov
Copy link
Contributor

I am already desperate.. monitoring the topic in general for more than 6 months already. I very much need pure promises in my code base, cause dealing with Signals and SignalProducers is just not OK.
Just request.then { map($0) }.then { consume($0) }.catch { ... }. Isn't this beautiful? Definitely better than .flattenMap(.Latest /*.Merge? .Concat?*/) { map($0) }.startWithResult { ... } And not that hard, as well - at least for minimal support.
10 time already have I almost added PromiseKit or some other promise library just for the sake of that code clarity, but every time decided not to, since putting another library which essentially does the same thing but with different syntax is something I can't afford.
Generally would be great to fully support the Promises A+ specs, but starting with something simple but robust and part of the core library covered by tests - that would be amazing!

@leonid-s-usov
Copy link
Contributor

leonid-s-usov commented Aug 13, 2018

Looks awkward, but maybe we could introduce here PromiseProducer which is cold and subject to replayLazily() ? Because Promise should be warm - well, if we are going for that name. Hopefully we are. You know, the one having .then() method ...

Edit: sorry about the replayLazily() above, it's not relevant here. I was thinking about "retry" semantics when added that note

@RuiAAPeres
Copy link
Member

RuiAAPeres commented Aug 13, 2018

tl;dr: Making another lib that leverages RAS underneath and implements the Promises A+ spec could be something I could do.

Hi @leonid-s-usov thank you for your input, we appreciate feedback regarding how users take advantage of ReactiveSwift and how we can enhance their experience while doing so.

Regarding your specific use case:

request.then { map($0) }.then { consume($0) }.catch { ... }`

/// versus

.flattenMap(.Latest /*.Merge? .Concat?*/) { map($0) }.startWithResult { ... }

It's important to notice a couple of things:

  1. ReactiveSwift already supports the then operator, which might make sense for your specific use case.
  2. Catch can be included as a convenient/sugar for a flatMapError.
  3. The mention of Merge? and Concact? leads me to believe (apologies if this is not accurate), that you are unfamiliar which their use case and in what situation one would make more sense than the other. For what you are trying to achieve, the .latest would be the intended one. This could be extended, like catch into a convenience method of your own.

As for the general idea of Promises, I don't think the inclusion of .once would sort your problems. I do believe, on the other hand, that another repository/library could help you achieve that. I am ok on doing that, since I don't believe it's a lot of work.

@leonid-s-usov
Copy link
Contributor

tl;dr: I would be happy to help with the Promises A+ layer over RAS.

To be honest, it still takes me a minute every time I get to choose between the latest, merge and concat, usually after some time of working on other, not reactive parts of my code. But I do know the difference and can always deduce the correct value eventually. I am actually using .latest for the situations.

My problem is that whenever I am trying to reinvent this Promise.then wheel with pure ReactiveSwift, neither of the values feel natural, because in this case I am not expecting to have multiple mapped producers!

And, reviewing the code of my colleagues I can see all three strategies (well, mostly two, the .latest and .merge) being used, and all of the variants actually working.

The actual .then operator is very much not what I need in the cases: it doesn't map the value of the signal it's applied to. This renders to two major problems: it doesn't receive the value as a parameter and it is required to have the same value type as the signal it's applied to. Actually I don't remember a single place where this operator was useful to me (just a side note, nothing personal).

I am actually having this task on my todo - implement a wrapper for ReactiveSwift having the Promise semantics. But you know, before getting there (which would be a fun task for sure) I was hoping that someone already did that or at least was planning to.
However I thought that such layer, if implemented "from within" RAS, could benefit from access to the internals of Signals and SingalProducers. Well, this is just a guess, I haven't yet actually played with this.

Lastly, I am not sure what you meant by

I don't think the inclusion of .once would sort your problems

I think it actually would be one step closer as that would give me (and anyone else) a special flatten strategy for the "promise chaining" use case, maybe even with some assertions to guard the "single event" expectation. It would be just disappointing to me because this route doesn't look like anything which would lead to the promises API

So, I'm totally for any initiative into the direction of having a well tested and concise Promise-like API on top of RAS. I've seen PromiseKit actually created a testing environment where they run the original JS A+ test suite on their implementation, isn't that cool?!

@RuiAAPeres
Copy link
Member

I think the way forward then would be open an issue here. Curious enough someone had a simillar idea RACCommunity/contributors#8

@leonid-s-usov
Copy link
Contributor

OMG thanks much!

@leonid-s-usov
Copy link
Contributor

I still see how any implementation would benefit from .once flatten strategy

@andersio
Copy link
Member Author

andersio commented Aug 13, 2018

My personal stance is that we should have a cold entity, that isn't named Promise for sure to avoid confusion.

I am sure that the warm behaviour of Promise is preferable in some cases, but having it cold by default allows us to: (1) have it stayed cold without needing another entity — a compact API is one of our strength; and (2) a cold entity can wrap warm semantics pretty intuitively (hello, replay operators!).

As for naming, there is a dissent around "inheriting" the name Single from Rx (by @mdiep, @sharplet and part of @andersio). That's the reason why names like Promise and Future are thrown around albeit not making much sense if we go after cold semantics.

After days of thoughts the best I can come up with is OneShot<T, E>. :p

@leonid-s-usov
Copy link
Contributor

leonid-s-usov commented Aug 14, 2018

What about a Promisor? Or Promiser... ;)

Still I can't easily get away from the kind of warm object which would actually hold the signal, and then eventually the result of the operation until the end of its lifetime. I mean, if we only have the cold one shot, then one would need to use some other layers to pass this eventual result on to consumers, if we want that the work is done on the background while we are sending the "yet to be prepared" result promise to all the interested parties. To me that's one of the important reasons to search for the kind of interface.

Let's be specific and take a simple example of a network request. I have some function inside some MyAPI class, called getRecentMessages(userId: String, limit: Int). If that one returns a cold OneShot object, then probably I'd need to change this method to

func configureGetMessagesWithParameters(userId: String, limit: Int) -> OneShot<[Messages], APIError> 

What would my code look like?

let request  = MyAPI().configureGetRecentMessagesWithParameters(userId: user.id, limit: 100)
request.executeWithResult { ... }

Well, this is not anything better than what I can achieve with SignalProducer today.
But let's say we can now flatmap it nicer

let loginAndGetMessages = MyAPI()
    .configureLogin(username: "peter", password: "pan123")
    .then {  MyAPI().configureGetMessagesWithParameters(userId: $0.id, limit: 100) }
loginAndGetMessages.executeWithResult { ... }

For that only... I'd be better off with the .once flatten strategy and that's it.

If we are for some actual development around this, I'd want to get rid of these ugly "configure" methods. I want to call

let user = MyAPI().login(username: "peter", password: "pan123")
let messages = user.then { MyAPI().getRecentMessages(userId: $0.id, limit: 10) }
user.thenOnScheduler(UIScheduler()) { [weak self] in self?.updateUser($0) }

Did you notice? I have used the user "promise" twice, but I didn't want it to login twice. So, how do we achieve this with a cold entity only?

I would agree that a cold entity can bring many useful things but we'd still have to have some promise-like thing that this cold entity resolves to. Just like SignalProducer resolves to Signal, this Promisor should make a Promise. Or whatever we call those things.

@leonid-s-usov
Copy link
Contributor

leonid-s-usov commented Aug 14, 2018

Maybe, it's not the cold object that we are lacking, but rather this Promise-like container which accepts a SignalProducer or a Signal in constructor, keeps the internal state of whether it has ever received the value or not, and supports the "then" semantics? Because, all the higher level tricks around replays and stuff can simply be done on the passed-in SignalProducers.

As its second task, the object may give the users some syntactic sugar to conveniently create a promise with a setup function which doesn't receive an observer, but rather some resolve callback.

Quite what the guys at OMsignal did in their ReactiveFuturesKit. Just don't pay too much attention to the class names there, the Promise they have used is very misleading. Their actual 'OneShot' class is the Future. Some highlights taken from Future.swift:

  • the essence:
    self.futureProducer = signalProducer.take(first: 1).replayLazily(upTo: 1)
  • the reason we are on this thread (note the strategy used):
    let transformedProducer = self.futureProducer.flatMap(FlattenStrategy.concat) { ...

@mluisbrown
Copy link
Contributor

mluisbrown commented Aug 14, 2018

My personal stance is that we should have a cold entity, that isn't named Promise for sure to avoid confusion.

I totally agree 👍 Promise would lead people to think you could use it just like a promise, which you can't. And it absolutely should be a cold entity.

As for naming, there is a dissent around "inheriting" the name Single from Rx (by @mdiep, @sharplet and part of @andersio).

Why is there dissent around using Single? Who cares if it's also used in Rx? I personally think Single is a perfect name, and much better than OneShot which seems like a contrived way to avoid using Single 🤷🏼‍♀️ If Single is the most apt name, then surely it's only an advantage that it's already used to mean exactly the same thing in Rx?

I also think that having Promise might make it easier for developers to transition from using Promise libraries like PromiseKit to ReactiveSwift.

I think that having talked about Promise was perhaps a mistake and has introduced some confusion about this proposal. IMO the objective here is, as the title states, a "Stream of one event", not something that can be used just like a Promise. If this proposal goes ahead, it could then be used as the basis for creating a true Promise object on top of it, but I believe they are separate issues.

Alternatively, the then operator could be overridden for this entity to provide the value, which would make it behave more like a Promise:

let loadUser: Single<User, NetworkError>

// ...

loadUser
.then { user -> Single<[Post], NetworkError> in 
    posts(for: user)
}
.then { posts -> Single<[Comment]>, NetworkError> in 
    comments(for: posts.first)
}

@leonid-s-usov
Copy link
Contributor

IMO Single, being a nice name on its own, would be a nightmare to spot between Signals in my interfaces....

@mluisbrown
Copy link
Contributor

How about Once, instead of OneShot @andersio ?

@leonid-s-usov
Copy link
Contributor

leonid-s-usov commented Aug 14, 2018

@mluisbrown
Regarding your example, could you please address the following questions / concerns I have risen above:

  • How does Single manifest its "coldness"?
  • When exactly does the action behind loadUser actually "fire", with loadUser representing a cold entity?
    Implicitly inside .then implementation?
    Or are you missing some .execute(onError: {...}) at the end?
  • the proper convention to the naming of methods which would return Single
    (should it be .getUserLoader(...) -> Single<User, Error>
  • how would I reuse the results of the loadUser to also update my UI once it's loaded, without actually logging in / loading again?

@mluisbrown
Copy link
Contributor

@leonid-s-usov

When exactly does the action behind loadUser actually "fire", with loadUser representing a cold entity?

Good point. You would still have to start it.

how would I reuse the results of the loadUser to also update my UI once it's loaded, without actually logging in / loading again?

Using replayLazily(upTo:)

@liscio
Copy link
Contributor

liscio commented Aug 14, 2018

I still don't really see the point of this, sorry.

A SignalProducer can already be used in places where you might instead use a Future or Promise. I have built dozens of async APIs that effectively use SignalProducer in place of Future<Value,Result> without having to take(last: 1), or use once(). That's because all my Future-style SignalProducers are defined to provide a single value and have a fixed lifetime.

I am currently wrapping up some work in an API that uses Futures heavily (Vapor+NIO), and I use SignalProducer internally in my controllers/etc because I'm much more comfortable with that construct and its concise error handling.

I also use SignalProducer in APIs that "terminate" with straightforward Delegate patterns:

let theProducer = internalController.someOperationWithParameters(/* ... */)
theProducer.startWithResult { [weak self] result in
  switch result {
    case .success(let value):
      self?.delegate?.didReceiveValue(value, in: self)
    case .success(let error):
      self?.delegate?.didFailWithError(error, in: self)
  }
}

Now, would I like some added comfort provided by the compiler and/or type system to guarantee that my SignalProducers only ever emit a single value? Maybe

Is it worth giving up the simplicity of having "two base types"[^1] that are vended by ReactiveSwift? I don't think so.

If folks are having trouble with explaining ReactiveSwift to teammates, it won't be any easier to also try and explain the subtleties of why you should instead use the Promise variant over a SignalProducer, how we bridge the two, etc.

Frankly, if you really want Promise<Value, Error> and Future<Value, Error>, then it should just exist in a whole new type (and probably even a separate library) that offers exactly that. IMO, there is far too much (conceptual and implementation) baggage and overhead that comes with Signal{,Producer} to use it as the basis of the simpler Futures & Promises construct.


[1] Sorry, Action—I rarely think of, or use you…

@leonid-s-usov
Copy link
Contributor

@liscio, I think you did a great job at bringing the discussion back to the topic. Given the title of this issue, I would agree with you that there doesn't seem to be much benefit from having another cold entity acting mostly like SignalProducer.

I personally came here in search of a solution to places where Promise semantics would make much more sense than returning SignalProducers

  • As you, I'm dealing with the existing capabilities of RAS just fine, but to me it does feel awkward to have my APIs and many other interfaces named and treated like someGetOperationWithParameters, which has to be started and replayed where necessary
  • Another place where I felt uncomfortable is the flatMap(.latest). I really regret now that .once FlattenStrategy pull request was closed, as that would be the efficient and unambiguous way to chain single event producers.
    However the same could be achieved with some other syntax, like by defining a separate operator chain (or chainFirst or chainSingle), which BTW we do have in our other project as an extension.

So, I was here to bring Promise into RAS, but you might be right, and this could be a wrong approach. And if not that, then what else would the "stream of one event" be which is not already a SignalProducer?

I will however invest some time into exploring whether there's some nice way of "terminating" a Signal(Producer) with a Promise-like Thenable entity, in a concise and unambiguous fashion.

@mdiep
Copy link
Contributor

mdiep commented Aug 14, 2018

Now, would I like some added comfort provided by the compiler and/or type system to guarantee that my SignalProducers only ever emit a single value? Maybe

Is it worth giving up the simplicity of having "two base types"[^1] that are vended by ReactiveSwift? I don't think so.

If folks are having trouble with explaining ReactiveSwift to teammates, it won't be any easier to also try and explain the subtleties of why you should instead use the Promise variant over a SignalProducer, how we bridge the two, etc.

Frankly, if you really want Promise<Value, Error> and Future<Value, Error>, then it should just exist in a whole new type (and probably even a separate library) that offers exactly that. IMO, there is far too much (conceptual and implementation) baggage and overhead that comes with Signal{,Producer} to use it as the basis of the simpler Futures & Promises construct.

I think it all depends on the specifics.

You raise some good concerns. But I think it's conceivable that this concept could be added in a way that actually makes ReactiveSwift clearer. It definitely needs to be considered holistically.

I think the Signal/SignalProducer distinction is confusing for people. I'm not sure it's the best division of responsibilities. So personally, I've been questioning what the right divisions and names are. I think that's the real question here: have we found the right model, or does it need to adapt slightly?

I agree that if this concept probably shouldn't be added if it can't be added in a way that clarifies the whole.

@andersio
Copy link
Member Author

andersio commented Aug 14, 2018

ReactiveSwift at a conceptual level is centred around Signal, which is refined by SignalProducer that is in turn further refined by Property. So we’ve already established a three/four level hierarchy for years that kinda qualifies for progressive disclosure. A cold stream of one (should we go after this path) adds a second refinement to SignalProducer, but it doesn’t break the basis of conceptual model formed around Signal.

I cannot dispute that it might in the end be a cognitive overload that offers little value over SignalProducer, other than one not having to choose parameters for several operators in the “I know this gonna be a stream of one” scenario. Especially, #516 makes the bar very high for it to be valuable no doubt.

While @mdiep raised a good point in the need of reviewing the very basis before we proceed to extend the framework, “stream of one” is IMO orthogonal to that discussion, since this particular refinement is valid regardless of the base, which should remain by default multi-value with explicit termination.

@liscio
Copy link
Contributor

liscio commented Aug 14, 2018

I can definitely envision a future (heh) where someone comes up with an adequate way to abstract a "one-shot, asynchronous operation" atop RAS' constructs, but I don't think that wrapping SignalProducer and calling it Promise or Future is the way to go about it.

Using a different approach, what about starting with something that aims to replace both the SignalProducer's error-prone construction as well as its one-shot result?

For instance, consider a type called Async<Value,Error>, with the following wrapper:

final class Async<Value, Error> {
  init(_ action: (Promise<Value, Error>, Lifetime) -> Void) {
    // Construct a normal SignalProducer, and use a new type, `Promise` to wrap the `send` API.
    // Then, call action() inside the SignalProducer's constructor with the wrapper
  }

  // This type serves only to wrap the `send` API on Signal.Observer.
  // Expectation could also be used instead of Promise to avoid confusion?
  struct Promise<Value, Error> {  
    func succeed(with value: Value) {
      // call send(value:) and sendCompleted() on underlying producer
    }
    func fail(error: Error) {
      // call send(value:) and sendCompleted() on underlying producer
    }
  }

  func await() -> Result<Value, Error> {
    // basically just invokes startWithResult on the underlying producer
  }
}

Usage would look something like this:

func doSomethingNetworky() -> Async<String, SomeNetworkError> {
  return Async { promise, lifetime in
    callSomeAsyncNetworkApi() { 
      if theCallSucceeded { networkStringValue in
        promise.succeed(with: networkStringValue)
      } else {
        promise.fail(error: SomeNetworkError.someError)
      }
    }
  }
}

let networkResult = doSomethingNetworky().await()

And of course we could add the requisite then, map, and so on APIs.

Because they still rely on an underlying SignalProducer, much of the existing code in the attached prototype would probably still apply.

Thoughts?

@leonid-s-usov
Copy link
Contributor

@liscio working on something pretty close to that, but I disagree with two things:

  1. You suggest wrapping a SignalProducer with a class called Async, then you suggest adding then, map and so on - effectively making it behave and quack like Promise - but you still insist that it's not the way to go
  2. Your use of Promise here is misleading, because Promise is a Thenable in the first place, and you only use it as a struct to hold the resolve and reject methods.

But in general I feel more or less on the same page. Let me just clear up these nasty type errors and I'll show a preview of what I mean.

@leonid-s-usov
Copy link
Contributor

OK people, please check out this draft #664
Inspired by PromiseKit

Some highlights:

struct Promise<Value, Error: Swift.Error>: Thenable, SignalProducerConvertible {
extension SignalProducer {
    func makePromise() -> Promise<Value, Error> {
        return Promise<Value, Error>(producer: self)
    }
}

func add28(to term: Int) -> Promise<Int, NoError> {
    return Promise { fulfil, _ in
        fulfil(term + 28)
        return AnyDisposable()
    }
}

let answer = SignalProducer<Int, NoError>(value: 14)
    .makePromise()
    .then { add28(to: $0) }

answer.await { (result) in
    print(result)
}
> fulfilled(42)

@andersio
Copy link
Member Author

andersio commented Aug 15, 2018

@liscio It seems you might have been mistaken here. The scope of “stream of one” has never been dead set on a warm Promise primitive.

This issue nominated a cold stream of one construct that is akin to Rx Single, and I think that’s what most of us have in mind. As mentioned, the “warm” start-immediately and replaying nature of Promise could more or less be covered by a replay operator on a cold construct.

Speaking of my own stance, I have no intention to expand the API surface to support another style of async API. The acceptance criteria for me would be a familiar but refined API similar to SignalProducer, and if it is cold, it should not be called Promise in any case absolutely. Most importantly, the most valuable thing we’d have on it is just parameter-less flatMap to faciliate a Promise “then” style composition. But this can be also exposed as flatMap(.once) without introducing a new construct.

@leonid-s-usov
Copy link
Contributor

@andersio
Although I can definitely see some points around having a Single-like specialisation of SignalProducer, I still think that it should go together with its warm Promise-like counterpart.
RxJava has Single.toFuture()
Correct me if I'm wrong, but RxSwift doesn't have too many goodies around Single, it's just throwing if the underlying observable is not keeping the "single value" contract, which IMO is just not enough to add the class to the API in the RAS case.

BTW, what do you think about ValueProducer and Value for the one shot specialised SignalProducer and Signal, having the Value actually behave like Promise?

@liscio
Copy link
Contributor

liscio commented Aug 15, 2018

@leonid-s-usov Sorry, I was operating under a more "complete" Promise definition that also includes a Future type. Something more like what's defined in swift-nio: https://github.com/apple/swift-nio/blob/d31f4a249a2c7eef373f044e781b2f89f7e5e64d/Sources/NIO/EventLoopFuture.swift#L150.

My preference is not to stomp on the Promise and Future naming because I don't think that anything that's been proposed so far is "sufficiently Promise-like".

Anyway, @andersio it's clear that I'm not quite following the purpose, which is why I'm grasping at straws to understand the "big picture" value of this type. When you name your prototype Promise, you should not be overly surprised when we're going to expect it to behave like whatever Promise library/definition we've encountered in the past… 😄

The true test would be to see how this type—however it is named—improves the readability/usability of ReactiveCocoa's extension on URLSession, the collect operation as suggested by @mdiep, and so on.

Giving us a handful of demonstrations of where it improves the call site / clarity will go a long way to help us understand what the ultimate goal is.

@mdiep
Copy link
Contributor

mdiep commented Aug 15, 2018

I do like the name Async quite a bit if we're not going to model promises/futures exactly.

@leonid-s-usov
Copy link
Contributor

leonid-s-usov commented Aug 15, 2018

Async tells nothing of whether it's a cold or warm entity. Also, it lacks the subject. If it was at least AsyncValue

And here again, I think that AsyncValueProducer and AsyncValue pattern could be well fitting the RAS type family

But, following the Property class name I personally like the simple Value and ValueProducer. Cause the async nature is kind of assumed if we consider the fully qualified name of ReactiveSwift.Value(Producer)

@leonid-s-usov
Copy link
Contributor

@liscio

@leonid-s-usov Sorry, I was operating under a more "complete" Promise definition that also includes a Future type. Something more like what's defined in swift-nio: https://github.com/apple/swift-nio/blob/d31f4a249a2c7eef373f044e781b2f89f7e5e64d/Sources/NIO/EventLoopFuture.swift#L150.

Well ok, another implementation which decided on such term usage (which I find less complete). Splitting roles between "Future" and "Promise" this way is not something I personally can get along with, but that's subjective, of course.
I believe it's totally incorrect, since Promise and Future are different names for the same concept, and whether one or another is chosen, it should be an entity providing all features: the await/chain interface and the resolve interface.

@liscio
Copy link
Contributor

liscio commented Aug 15, 2018

@leonid-s-usov Yep, totally subjective, and I should probably not have used the word "complete".

The reason that it makes more sense to me is that—at the call site—you see that you are getting a value of a particular type in the Future. In contrast, one could argue that the alternative is that the function is returning a Promise to return that value (in the future). In both cases, you're still getting something that will "resolve in the future", hence my preference for Future as the "key type".

From there, it follows that—in order to return/create a Future—it makes sense to create a Promise that you will then fulfill within the context of the function (whether it happens as a result of an async call, or immediately, doesn't really matter.)

At any rate, this is obviously something that's interpreted in many ways, as the wikipedia page for this topic is quite dense.

Now, I have no idea why you think that Async or any other Future-like construct needs to communicate whether it is warm or cold? Why on earth would that matter if what we're trying to do is build something that is designed to (always) return a value/error in the future, and be easier to understand?

One thing you left out in your "calling by its name" is the fact that Value is right there in its generic parameters: ReactiveSwift.Async<Value, Error>. In actual use, it'd be ReactiveSwift.Async<Int, SomeError>, or Async<String,SomeError>, etc.

Anyway, this is way off-course from the original discussion now because this issue is not really trying to build a Future/Promise type, but rather a replacement for Rx's Single. While I agree that Single / Signal might be easy to transpose while typing, I think that the strength of the meaning of Single outweighs that issue.

@RuiAAPeres
Copy link
Member

Now, I have no idea why you think that Async or any other Future-like construct needs to communicate whether it is warm or cold?

💯

@mluisbrown
Copy link
Contributor

Anyway, this is way off-course from the original discussion now because this issue is not really trying to build a Future/Promise type, but rather a replacement for Rx's Single.

👏

Can we please forget about Promise and Future for this discussion (notwithstanding the naming of the prototype)?

As @liscio said, this issue is about creating an equivalent to Rx's Single.

While I agree that Single / Signal might be easy to transpose while typing, I think that the strength of the meaning of Single outweighs that issue.

💯

Absolutely agree. I don't see any reason why RAS should not also adopt the Single name.

@leonid-s-usov
Copy link
Contributor

@liscio
I totally follow the motivation behind both Future and Promise, what I'm actually against is mixing them in one implementation. Historically both of these were designed as fully independent concepts so I tend to stick to that... But enough of that.

ReactiveSwift.Async<Int, SomeError> is cool, agreed.
However ReactiveSwift.AsyncValue<Customer, NetworkError> isn't that bad as well, IMO

Well, the question about the cold / hot nature of the entity has been risen above, and not originally by myself. If you followed this from the beginning I was all for the warm entity - that is, the one where the operation starts behind the scenes and we can just consume the result.

However, following comments from @andersio I agree that it would be great to have control - when needed - over when the background operation actually starts, and that would be pretty much in line with the current interplay between SignalProducer and Signal. This clarity is actually stated as one of the major concepts behind RAS, so I don't quite understand why you say its so unimportant. Hope that it's not just because of the naming.

And Single... c'mon, I believe we can do better than that. We all seem to agree here that having this kind of functionality in RAS is a good thing, but I'd avoid such clash in class names, especially given that RAS has so few of the core classes for now.

@liscio
Copy link
Contributor

liscio commented Aug 15, 2018

@leonid-s-usov You're helping to strengthen my original comment of, "I still don't really see the point of this." 😄

Here's what I'm gathering (roughly) from the bulk of this thread…

We want a producer that returns only a single value, or an error.

Well, we have take(first: 1), take(last: 1) for that. Or, you create a SignalProducer in a way that explicitly sends a single value and completes immediately after.

We want to get the value, similar to how Future or Promise works

We offer single(), last(), and first() to perform those functions, the latter two combining the above concept with single.

We want to distinguish hot vs cold

OK, we have Signal vs SignalProducer.

We want control over the operations

This is why we have so many variations of take, start(on:), observe(on:), and so forth.

We want a nice type to encapsulate a special case of the above

This is proving to be very hard to achieve, because there doesn't appear to be a winning special case that contains the most desirable combination of the above.

@leonid-s-usov
Copy link
Contributor

leonid-s-usov commented Aug 15, 2018

I think you are missing some points, let me continue your list

We want code clarity when the returned producer will only fire once or fail

There is no way to achieve this today. Interfaces are either semantically incorrect

func getMessages(userId: String) -> SignalProducer<[Message], ApiError>
...
api.getMessages(userId: "123").start()   // why start?? 

or ugly

func configureGetMessages(withUserId userId: String) -> SignalProducer<[Message], ApiError>     // (x_x)
...
let messageGetter = api.configureGetMessages(withUserId: "123")  
messageGetter.start()       

We want a clearer (and more efficient) flattenMap strategy when chaining streams of single value

The "correct" answer to this today is to use flattenMap(.latest) { ... } which is counterintuitive when you are thinking about single value streams

We (well, at least myself, a user of RAS) want a promise like functionality to avoid adding another library; to have interoperability between the provided promise and SignalProducer/Signal to utilise convenience functions and natively convert between interfaces within one framework

Well, a lot has been said about this one. Even a Promise draft.

We want other single value oriented operators, which would be more efficient, readable and safe for this common use case of real life reactive programming

(@andersio please help me here)

@liscio
Copy link
Contributor

liscio commented Aug 15, 2018

@leonid-s-usov We're effectively on the same page. When I said, "We want a nice type," I meant it in a way that's equivalent to "We want code clarity[…]"

Regarding your "semantically incorrect" example, I am not sure you provided enough context to explain where the problem is. For instance, I see all three of the below as having a similar meaning:

func getMessages(userId: String) -> SignalProducer<[Message], ApiError>
func getMessages(userId: String) -> Task<[Message], ApiError>
func getMessages(userId: String) -> () -> [Message]

I read any of the above as, "give me a way to execute getMessages in a deferred manner." Whether you have to call start() to begin the process, or startWithResult(), or last() to get the value at that point is a detail of the underlying API.

Maybe I'm missing something about how the returned producer is "hooked up" elsewhere to pass values along?

Regarding a new flattenMap strategy, I think that's a separate issue. If we get a new type that provides clarity, then we shouldn't need alternative strategies for the existing types.

@leonid-s-usov
Copy link
Contributor

leonid-s-usov commented Aug 15, 2018

Well, my point is that all of the methods above should have been called

func messagesGetter(forUserId: String) -> ...

which would make my OCD happy and I would know that whatever value returned is a "generator" which must be activated before any values are to be expected

In contrast, what I would love to have is

func getMessages(userId: String) -> Async<[Messages], ApiError>

where the returned value actually represents the result - but in this case, an asynchronous one. Or maybe, by the time we got to check it, the result could already be there. Or, it was returned from cache and can be used straight.

@leonid-s-usov
Copy link
Contributor

leonid-s-usov commented Aug 15, 2018

Regarding a new flattenMap strategy, I think that's a separate issue. If we get a new type that provides clarity, then we shouldn't need alternative strategies for the existing types.

It used to be a separate issue, but was closed in favour of this one

@andersio
Copy link
Member Author

andersio commented Aug 15, 2018

@liscio:
Now, I have no idea why you think that Async or any other Future-like construct needs to communicate whether it is warm or cold?

As @mdiep mentioned the need of reviewing our base (Signal/SignalProducer), I tried to gather my thoughts around it. It turns out that my arguments followed this very same path of logic (link to a manifesto).

It does pose an interesting doubt on whether a separate Single type would offer substantial value after all. Looking at the Single type in the same manner as in the manifesto:

  1. From an API consumer perspective, it has no difference from SignalProducer at static time.

    Single promises only to notify its observer once at runtime. At static time, the guarantee can be no stronger than any other async construct (i.e. maybe invoked multiple times), because this is the nature of scope-escaping closure/callback patterns in general. In other words, Introducing a new type that provides no extra astatic time guarantee seems dubious.

  2. From an API producer perspective, it does in theory provide a more concise set of API. A notable example is that flatMap would be strategy-less, and a few operators would be dropped since they no longer make sense given the runtime contract of only one event.

    But since it barely has any effect on expressivity of intent, it becomes a question of whether mere brevity should be weighed against a considerably larger API surface (which incurs maintenance cost), plus zero extra static guarantee.

(These are akin to the questions I had asked in #201.)

@leonid-s-usov
Copy link
Contributor

leonid-s-usov commented Aug 16, 2018

@andersio So it looks like few people already agree that from the api consumer perspective a simple cold Single type alone will not be worth the trouble, even if we take into the consideration some added benefits to the api producer.

However IMO it will make much more sense if it will come with its warm promise-like counterpart. For the sake of this message let me temporarily pin the names: what you previously referred to as "cold Single" i'll call AsyncValueProducer and its warm counterpart (with benefits like caching the value and Promise semantics) - an AsyncValue

With these in place we can focus on using the AsyncValue in cases where a simple asynchronous api response is needed (or similar tasks), and AsyncValueProducer may be used to receive a number of things which are served cold: replay and retry and probably something else I can't remember now.

@leonid-s-usov
Copy link
Contributor

I wonder what this silence means.

Maybe this topic should be re-launched from scratch in a new issue, with some summary of this discussion like the one we had few messages above?
Cause I doubt there will be anyone with enough courage to go through all 40 verbose comments.

WDYT?

@leonid-s-usov
Copy link
Contributor

Please check out the new issue #667. I really hope it will facilitate the resolution of this thread - whatever the final decision will be.

@leonid-s-usov
Copy link
Contributor

There's another POC implementation I've posted in #668

@leonid-s-usov
Copy link
Contributor

Anyone?

@jjoelson
Copy link
Contributor

In #668, it seems the example would look almost exactly the same with a SignalProducer except the need to provide a flatten strategy is gone, on has been renamed when, and start has been renamed await. Isn't a major goal of a framework like ReactiveSwift to have a common vocabulary of types and operators that can represent streams of values over time? Bracketing off a particular subset of use cases and changing all of the names seems counter to the mission as I understand it.

The ability to have flatMap without a flatten strategy is a real gain, and I'd like to understand what other improvements like that can be had by introducing a new type to represent a single value stream. Apart from things like that, I think the public API for this new type ought to look as much like SignalProducer as possible.

@leonid-s-usov
Copy link
Contributor

@jjoelson thanks for the feedback.

If you don't mind, let's continue this discussion in #667 . That is a new issue I have created as an essence of this thread, and it has a convenient list of all properties of the potential new API. I will copy this comment there.

I would agree with on/when, but start / await have different meaning in terms of the expected number of invocations. Also, await has a side effect of actually starting the producer if called on an instance of AsyncValueProducer, but doesn't start anything if called on AsyncValue.

Having that said, personally I'm not married to any naming. My only strict criteria is that I believe if we choose to change something we need to be consistent and not create a mixture of patterns.
I think that if there is any functional difference in otherwise similar methods, these methods should be named differently.

@leonid-s-usov
Copy link
Contributor

@andersio there is a work-in-progress badge here. Any actual progress?

@RuiAAPeres
Copy link
Member

Hello. 👋 Thanks for opening this issue. Due to inactivity, we will soft close the issue. If you feel that it should remain open, please let us know. 😄

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

No branches or pull requests

7 participants