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

Promise draft #664

Closed
wants to merge 4 commits into from
Closed

Conversation

leonid-s-usov
Copy link
Contributor

@leonid-s-usov leonid-s-usov commented Aug 15, 2018

Not really a pull request, but rather a dirty draft in a sandbox, for review.
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)

@leonid-s-usov
Copy link
Contributor Author

@RuiAAPeres WDYT?

@RuiAAPeres
Copy link
Member

Need to spend some time on this, but my initial feeling is that I don't fully agree with this direction.

I assumed the goal was to abstract a SignalProducer via another Layer (Promise). If you make it comply with the SignalProducerConvertible protocol, I don't see any advantage on this extra layer.

@leonid-s-usov
Copy link
Contributor Author

The SignalProducerConvertible is there to be able to use all those convenience functions defined to accept objects adopting this protocol. At the end of the day, the Promise is built on top of a producer and can easily become one for the sake of polymorphism.

It's strange that this simple compliance is changing your attitude. It's pure convenience.
In any case, one may always want to convert a "OneShot" entity to a SignalProducer, so why not manifest this with the "Convertible" interface?


var lifetime: Lifetime { return _lifetime }

var producer: SignalProducer<Value, Error> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be SignalProducer.init(_: Promise<Value, Error>) instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, agreed. state can be made fileprivate and an extension for SignalProducer could be defined. Also it would be cleaner to make this a let property cause there's no reason in creating a new signal producer for every caller

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -22,4 +22,186 @@ import Foundation
A place where you can build your sand castles 🏖.
*/

enum PromiseResult<Value, Error: Swift.Error> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could just use Result for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well that was my intention in the first place, but then I couldn't work around the case with nil error.
That's actually a questionable decision to store it this way, other implementations chose to wrap a user error into additional error enum like

enum PromiseError: Error {
     case nested(error)
     case interrupted
}

In this case standard Result would do. I'm open for suggestions here - including an option to fatalError the case of interrupted / empty source producer. Which is of course not something I'd agree for ;)

}
}

protocol Thenable {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we'd get any benefit from this protocol.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah.. I just snatched it from PromiseKit, after reading the specs which refer to Thenable as something separate from the Promise itself. Technically, there could be thin auxiliary classes implementing the protocol directly, without the overhead of the full Promise

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have refactored the code, and now Thenable is a type-erased ThenableType, which allows for a number of nice optimisations

@RuiAAPeres
Copy link
Member

RuiAAPeres commented Aug 15, 2018

It's strange that this simple compliance is changing your attitude. It's pure convenience.
In any case, one may always want to convert a "OneShot" entity to a SignalProducer, so why not manifest this with the "Convertible" interface?

The goal of having a Promise is to hide the apparent "complexity" of a SignalProducer/Signal behind an easy to digest API. From the moment you add SignalProducerConvertible, you are opening the pandora box, so the big advantage (simplicity) is lost. I fear I have completely misunderstood your intentions/ambitions/motivations with this new entity (which for the record, is completely fine).

It would be interesting to see how many people, besides you, would be interest in using it like that. I think this is a case of less is more and fundamentally why initially I was onboard on this new entity.

@leonid-s-usov
Copy link
Contributor Author

Sorry, I still can't see what it changes. How can the fact that Promise is convertible back to SignalProducer compromise its simplistic API?

Let's for a moment remove this compliance. Do you agree that there should be a way to create a SignalProducer out of a Promise, given that these two are part of the same framework? Just like you have this ability with Signal and Property, both can be rendered into a SignalProducer.

If yes, then that's it, you take a Promise, convert it to a SignalProducer and you have the same pandora box. So what is different if there is no explicit conformance to SignalProducerConvertible?

I mean.. I don't really mind consider removing that protocol, I just want to understand. And it's a pity to hear that you "completely misunderstood my original intention", cause I feel like I have achieved exactly what I wanted. I have a promise now, I can construct it from scratch or convert a SignalProducer to it, I have the promise-like behavior and there's nothing that stops me from even implementing the Promises A+ (apart from the time, of course)

@andersio
Copy link
Member

ref: #668 (comment)

@andersio andersio closed this Nov 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants