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

add the 'do notation' function to Future #8

Merged
merged 1 commit into from
Aug 23, 2018

Conversation

RPallas92
Copy link
Contributor

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

@emmanueltouzery
Copy link
Owner

Great, thank you for the contribution! 👍

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
Copy link

Nice addition

@RPallas92
Copy link
Contributor Author

RPallas92 commented Aug 14, 2018

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
Copy link
Contributor Author

RPallas92 commented Aug 14, 2018

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

@qm3ster
Copy link

qm3ster commented Aug 14, 2018

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
Copy link
Owner

emmanueltouzery commented Aug 14, 2018

@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
Copy link
Owner

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
Copy link

qm3ster commented Aug 14, 2018

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 emmanueltouzery mentioned this pull request Aug 14, 2018
@emmanueltouzery
Copy link
Owner

I opened #9 to continue the laziness debate. Let's keep this PR discussion about the lazy constructor from promises.

@emmanueltouzery
Copy link
Owner

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

@emmanueltouzery
Copy link
Owner

renamed #9 to "Future api changes". here would be just about the building of Futures, which is a smaller debate.

@qm3ster
Copy link

qm3ster commented Aug 14, 2018

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
Copy link
Owner

regarding the possible constructors suggested by @qm3ster:

  • ofPromise/of: no issue, it's just a matter of naming
  • ofCallback: interesting, I wasn't aware of that node API pattern. I confess that I've stuck to sync versions of node fs functions. Is this pattern node-specific, to the best of your knowledge? Thinking about naming, again... could name it ofNodeCallback for instance, if this was in fact known as a 'node' pattern.
  • liftBBBB: interesting, didn't realize it was possible to lift a function no matter the number of parameters it takes or even optionality of parameters and so on. But the interface you gave doesn't compile, I'm not sure of the implementation -- will skip this one for now except if you maybe make a PR. Can always be added later anyway.
  • lazyPromiseConstructor: that's the same as the current ofCallbackApi, except I didn't put a reject callback because it felt wrong to have two channels to signal failure (reject and throwing), and because we can never prevent throwing. But your argument about throwing being synchronous while reject can be called async, plus the advantage of being the same API a new Promise make clear that not adding the reject call (which can be skipped anyway in a type-safe way) was wrong.

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).

@emmanueltouzery emmanueltouzery mentioned this pull request Aug 19, 2018
@qm3ster
Copy link

qm3ster commented Aug 20, 2018

  • ofPromise/of: THE POWER OF DEFAULTS

  • ofCallback: This style is all over node core and early node libraries, almost nowhere on the web. Callback-last is fo I usually use fs-extra for fs, which is promisified out of the box.

  • liftBBBB: I think it only started typechecking in 3.0 Perhaps it's because the example:

    • used lift instead of liftBBBB
    • used WebAssembly (often not in scope)

    I edited it now, should be fine.
    A naive implementation could be as simple as:

    const liftBBBB = fn => (...args) => Future.of(() => fn(...args))

    A better(?) one might hold a reference to the args and fn inside the Future object for one less closure.
    In either case, the important optimization is to drop the references after the function is called.

@emmanueltouzery
Copy link
Owner

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
Copy link

qm3ster commented Aug 20, 2018

@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
Copy link
Owner

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
Copy link

qm3ster commented Aug 20, 2018

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
Copy link
Owner

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
Copy link

qm3ster commented Aug 20, 2018

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
Copy link
Owner

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 emmanueltouzery merged commit d74615d into emmanueltouzery:master Aug 23, 2018
@emmanueltouzery
Copy link
Owner

@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
Copy link
Contributor Author

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

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

Successfully merging this pull request may close these issues.

4 participants