emmanueltouzery / prelude-ts

Functional programming, immutable collections and FP constructs for typescript and javascript
ISC License
377 stars 21 forks source link

add the 'do notation' function to Future #8

Closed RPallas92 closed 6 years ago

RPallas92 commented 6 years ago

Added a "do notation" function in order to use async/await without caring to add try/catch blocks

emmanueltouzery commented 6 years ago

Great, thank you for the contribution! :+1:

Interestingly i had planned another function with the exact same signature, but named ofLazy or ofLazyPromise. In the end this do is very similar to of. But I agree that it's needed. My reasoning for needing it was a little different: Future, unlike Promise, can be lazy, but it gets you almost nothing if when creating from a Promise you can only be eager, like Future.of, since the Promise is anyway already started.

So we are adding the function, 100%. The only question is the name to use.

Well, actually my version was a little different than yours:

 static ofLazyPromise<T>(promiseProducer: ()=>Promise<T>): Future<T> {
     return new Future(Lazy.of(promiseProducer));
 }

So, my version is lazy, yours is eager, but the signature is the same. The lazy one won't be started until it's awaited on, or in general read (with then or something else).

Right now I prefer the lazy one, the naming question remains.

josete89 commented 6 years ago

Nice addition

RPallas92 commented 6 years ago

Hi Emmanuel, thanks for the response!

I agree with you that lazy one is better than mine 👍

Regarding naming question, I am not sure 100% what fits better for this case. I chose "do" because it was familiar to me as is the same in Haskell, but maybe ofLazyPromise is better.

RPallas92 commented 6 years ago

Maybe Future.lazy or Future.fromLazy or just Future.of

qm3ster commented 6 years ago

A bit off topic, but why are .map and .flatMap eager? Seems like it defies a lot of the benefit of laziness. Seems like only .then should trigger the computation. Also, surely .toPromise is eager, perhaps that should be documented.

emmanueltouzery commented 6 years ago

@qm3ster good point! Well if you use await then implicitely then is called and so your promise is triggered. In that scenario we don't need map & flatMap to be eager, they can be lazy.

we also have onComplete, onFailure, onSuccess, I think it's clear that these should trigger the future, so be eager.. So again map & flatMap could be lazy if you end by an onComplete & so on.

But I was a little unsure about map & flatMap being used standalone. I may yet change this, I'm afraid that people would expect the future to be triggered and it's not. People will likely do side-effects also through Future.map, as they do today in Promise.then, and expect them to be triggered. I probably need to read some more about the monix Task which is also lazy, and how they solve this. If you have some special input to give as part of this, maybe open a new bug. I'm definitely interested in all input!

[agree about documenting that toPromise is eager, will do]

emmanueltouzery commented 6 years ago

Note that I'm thinking about minor behaviour changes to onComplete/onFailure/onSuccess so now would be a great time if we want to change some other things. Unsure what to do though. Future is still brand new, so I'm definitely open to changes, including relatively large ones.

qm3ster commented 6 years ago

I think only things like forEach ( in this case onSuccess ) and collect ( in this case, await or toPromise ) should be eager. Nothing that passes the future through transformations should trigger the computation. It's very unexpected for filter and map to be so different from each other. I'd go so far as to say onXxx shouldn't even be eager, as they are written in EventListener on vocabulary. So some other thing, whether .then(callback) or even .trigger() should need to trigger the evaluation, and the .on should just observe the immediate future. This way, if it's pre-resolved the on is triggered in set immediate, but if it's untriggered it lies in wait. This is what you'd expect from other lazy types such as streams.

emmanueltouzery commented 6 years ago

I opened https://github.com/emmanueltouzery/prelude.ts/issues/9 to continue the laziness debate. Let's keep this PR discussion about the lazy constructor from promises.

emmanueltouzery commented 6 years ago

aha but we can discuss naming like of & ofPromise & ofCallback here. We can still make these changes at this point yes.

emmanueltouzery commented 6 years ago

renamed https://github.com/emmanueltouzery/prelude.ts/issues/9 to "Future api changes". here would be just about the building of Futures, which is a smaller debate.

qm3ster commented 6 years ago

For best ergonomics, I think the following constructors are needed:

// triggered beyond our control
declare const ofPromise: <T>(promise: Promise<T>) => Future<T>

// the spiciest, accepts `async` functions directly
declare const of: <T>(fn: () => Promise<T>) => Future<T>

// Provides node style callbacks. Throwing like `ofCallbackApi` isn't ergonomic,
// because you can only throw synchronously, but defer resolving.
// Basically equal to `of(promisify(fn))`
/* Example: */ ofCallback(cb => fs.readFile('/foo.txt', cb)) // is better
/* than writing */ of(() => promisify(fs.readFile)('/foo.txt'))
declare const ofCallback: <T>(cb: (err: any, result: T) => void) => Future<T>

// Don't know how to name it, but the benefit is obvious
declare const liftBBBB: <A extends any[], T>(
    fn: (...args: A) => Promise<T>,
) => ((...args: A) => Future<T>)
// Example:
const prefix = liftBBBB(async (inp: string) => '_' + inp)
// typeof prefix === const prefix: (inp: string) => Future<string>

// Same signature as `new Promise()`
// for when you want to write a promise by hand
declare const lazyPromiseConstructor: <T>(
    executor: (resolve: (T) => void, reject: (any) => void) => void,
) => Future<T>
emmanueltouzery commented 6 years ago

regarding the possible constructors suggested by @qm3ster:

So, I'll think about naming, especially if Future won't be lazy anymore. Will think about it (and comments will be possible in my PR if not before).

qm3ster commented 6 years ago
emmanueltouzery commented 6 years ago

ok, for Future.lift, I had to adapt a little your code because I'm using a class for Future, but it does seem to work very well:

    static lift<T extends any[],U>(fn: (...args: T)=>Promise<U>): (...args:T)=>Future<U> {
        return args => Future.of(fn(...args));
    }

I don't think there's a need to do anything regarding args because it takes a function returning a Promise that should return immediately anyway.

So this looks good, however I think I'll delay a little the inclusion of this function in prelude.ts, because I don't want to rely on too new versions of typescript. Already when I added use of Exclude I waited so that the feature is supported on the two latest typescript releases. So I'd rather add Future.lift only when typescript 2.1 gets released. But this certainly has implications also for FutureX.liftOption, FunctionX.liftEither and so on that prelude has... I'll think about this more when TS 2.1 gets released but for sure something will change there since TS now gives us this feature.

Regarding the rest of your comments, you can see the PR that I made. Thank you again for your comments!

qm3ster commented 6 years ago

@emmanueltouzery I think lift would be a very bad name for this, hence my stupid BBBB name. I still can't seem to find a proper name. a true lift of T=>U would produce a function Future<T>=>Future<U> (which inside would do fn => future => future.map(fn))

emmanueltouzery commented 6 years ago

hmm damn you're right! I was using lift terminology wrongly, including in other spots in prelude :-( i guess 'futurify' would keep with the 'promisify' trend :) but I'm not crazy about that and I need a name also for the other contexts that I have, like liftEither, liftOption. I'll think about it, but no rush since I want to wait for TS 2.1 for that function.

Thank you for the very valuable feedback!

qm3ster commented 6 years ago

You could include it for just unaries. Or just unaries and binaries.

// Defined with overloads
function dift <T1,U>(fn:(a:T1)=>Promise<U>):(a:T1)=>Future<U>
function dift <T1,T2,U>(fn:(a:T1,b:T2)=>Promise<U>):(a:T1,b:T2)=>Future<U>
function dift <T1,T2,T3,U>(fn:(a:T1,b:T2,c:T3)=>Promise<U>):(a:T1,b:T2,c:T3)=>Future<U>
function dift <T1,T2,T3,T4,U>(fn:(a:T1,b:T2,c:T3,d:T4)=>Promise<U>):(a:T1,b:T2,c:T3,d:T4)=>Future<U>
// Body from 1986
{
  return function () {
    var args = arguments
    return Future.of<U>(function () {
      return fn.apply(undefined, args)
    })
  } as any
}

// Still works with arrow functions, despite `arguments` usage
const lool = dift(async (x: number, y: number) => x + y)
emmanueltouzery commented 6 years ago

We could have an overload version for now, good idea, but I'd rather wait a little for the real thing, typescript releases are very fast recently.

For the name.. Future.convertFunction maybe? There should be 'function' in the name, I think. Or a shorter Future.convertFn.

qm3ster commented 6 years ago

You can just retype this function later, not breaking any code, just breaking compatibility with very old tsc. What do you mean by "wait" for the real thing? What tsc version are you maintaining compatibility with?

emmanueltouzery commented 6 years ago

well I would have rather waited just to avoid doing the work twice and also a little due to concern of potentially breaking some client code but I guess it's really safe, and also you've basically given the implementation. So we could include it in the next release then, the only thing we need is a name. I was thinking about Future.convertFunction or convertFn? Or maybe convertPromiseFn, but...

I wanted to look how they name this in scala-land.. and it turns out... they say it's lifting if I understand correctly: https://stackoverflow.com/questions/17965059/what-is-lifting-in-scala#comment26260243_17965570

Remember a PartialFunction[A, B] is a function defined for some subset of the domain A (as specified by the isDefinedAt method). You can "lift" a PartialFunction[A, B] into a Function[A, Option[B]]. That is, a function defined over the whole of A but whose values are of type Option[B]

On the other hand for the lifting when all parameters are lifted besides the result, they say:

"lift" the function A => B into the domain of the functor. "lifting into a functor"

It is there => https://www.scala-lang.org/api/current/scala/PartialFunction.html#lift:A=%3EOption[B]

PartialFunction.lift would seem to be similar to what we are doing. Also see: https://stackoverflow.com/questions/28993794/how-to-lift-a-partialfunction-to-either

So that would mean lifting is a general concept, of which functor lifting is only a sub-category.

On the other hand this one says lifting is only "functor lifting": https://stackoverflow.com/a/43596202/516188

So, maybe lifting is an appropriate naming after all, or maybe not. Maybe only the scala community is abusing the terminology, or maybe there's a difference between what they do and what we want to do here. I am quite unsure :-(

And then I come back to the names I mentioned with convert, but they don't really convince me. Maybe I could ask on stackoverflow about the lifting subtleties (is it only functor lifting, or not).

Otherwise in terms of typescript version compatibility, no hard rule, but I'm aiming to supporting the latest two versions. So currently that would fail because the better way you suggests fails on 2.9. Once 3.1 is released, it would work on 3.1 & 3.0, so the two latest versions, and I would include the better version.

emmanueltouzery commented 6 years ago

@RPallas92 you could have easily missed it in all the noise but in the end your PR was merged, so in master there is now a Future.do! It'll be in the next release (I think probably still over a week away though). Thank you for the PR!

RPallas92 commented 6 years ago

@emmanueltouzery many thanks! This lib is very useful for me.