Future API changes

[emmanueltouzery]

continuing the discussion from #8

@qm3ster => all good points, food for thought. Note that prelude.ts has a Stream collection type and yes it works as you described. I would consider renaming the on* methods and have them possibly returning void instead of returning the Future, but I'm not sure for the name right now (maybe onFailed=>ifFail, onSucceeded=>ifSuccess, onCompleted=>match or matchValue), or even if that's really the good path forward.
That's also connected to some changes I'm working on in these methods regarding error handling.

And besides that I think yes probably map & flatMap should be lazy, and we can also add an explicit. .trigger() (with in apidoc that you don't need it if you do await, or if you do X or Y and so on). The trigger would likely return void.

[qm3ster]

I think the .onX should be lazy and return the (same reference) Future for fluency.
Now:

aFuture
aFuture.onSuccess(cb) // <- triggered here
await something
aFuture.onError(cb2)

aFuture
aFuture.onError(cb2) // <- triggered here
await something
aFuture.onSuccess(cb)

What I propose:

aFuture.onSuccess(cb).onError(cb2)
await something
aFuture.trigger()  // <- triggered here

and:

await aFuture.onSuccess(cb).onError(cb2) //👌

[emmanueltouzery]

clearly my worry is that someone forgets to call trigger. I'm thinking about adding some constructor Future.run, or renaming Future.of to Future.run. But I must think it through.

[qm3ster]

On the other hand, I can imagine people misusing Future overall.

const datas = Promise.all(['url','url2'].map(fetch))  // the downloading will begin here
someStuff()
await something
someMoreStuff()
useDatas(await datas) // the datas are ready

but now

const datas = Future.sequence(['url','url2'].map(Future.liftBBBB(fetch)))
someStuff()
await something
someMoreStuff()
useDatas(await datas) // the downloading will only begin here

What is bound to happen if this lib becomes popular is that async makes my code go fast people will graduate to laziness makes my code go fast people, with further performance degradation.

[emmanueltouzery]

To be honest it was borderline for me whether to make Future lazy or not. In the end there is still the option of having Future eager, and if you want a lazy version, you can use ()=>Future. Any opinion about that?

One use-case for Future being lazy was:
Future.traverse(list, fetch, {maxConcurrency:3})

and I actually coded this (will push this soonish). But actually this does not require Future being lazy, since we can build the futures on the fly within Future.traverse!

What would require them being lazy is:
Future.sequence(futures, {maxConcurrency:3})

But we could have the feature only on traverse... Lazy futures have other advantages of course, but I don't want to cause too much confusion..

[qm3ster]

someone forgets to call trigger

🤷‍♂️ such is the price of laziness.

There's no TypeScript or even runtime way to warn people that their Futures aren't being triggered, because building up and then not triggering a pipeline is a valid usecase.

Perhaps there should be a Future and a LazyFuture class, with all the same convenience methods.
But then, what happens when people shove a mixture of them into firstCompletedOf? liftA2? sequence?
Perhaps the output should depend on where they got the static method from, LazyFuture or Future constructor.

[qm3ster]

I think there are valid usecases for lazy future pipelines if we consider that LazyFutures are to Streams as Options are to Arrays. Just like a Future pipeline can shortcircuit on a failure, it can also conditionally never get triggered (for example because another Future going into the same liftA2 or sequence already rejected.

Theoretically, the stream usecases don't actually need a LazyFuture, unlike what I originally thought:

declare const awaitEither: (Stream<Future<T>>)=>Stream<Either<T>>
declare const awaitOption: (Stream<Future<T>>)=>Stream<Option<T>>
declare const awaitSuccess: (Stream<Future<T>>)=>Stream<T>

As long as the source stream lazily produces eager futures on pull, that's perfectly sufficient.

[emmanueltouzery]

relevant, because monix Task is lazy =>
https://monix.io/docs/2x/eval/task.html#execution-runasync--foreach

Task instances won’t do anything until they are executed by means of runAsync

Which is exactly what you were saying. And you're right, this is more sane. So, what we don't want is what we have now, complicated mechanisms, "the future will be forced if you call map, or if you call onComplete, but not if you call filter".

There's one "but" though. Because if we build from a Promise, it's already started anyway.

I also think we don't want both Future and LazyFuture, at least not for some time. So I think we should pick one.

So I see two options:

1. Future is lazy. You must call runAsync() (or some other name) to trigger it, or call await on it (which will call then behind the scenes). If you give it something which was already started then that was already started, but the map & stuff you'll apply on it won't actually be executed unless you call runAsync (or whatever we call it). And also getPromise() triggers it.

2. Future is eager. Things are simpler, we do loose some elegant scenarios, on the other hand in prelude.ts we have Lazy and Stream, plus you also have the option of ()=>Future<T>

What we've established now is that the fact that the user must be well aware that the Future is lazy. At this point I'm reconsidering, thinking maybe we should go for the simple solution, an eager Future like what most people are used to. After all this is what you get in scala, in vavr. Unsure about this right now.

[RPallas92]

Good discussion! What scenarios are we loosing with solution 2?

[emmanueltouzery]

@RPallas92 well if Future is eager, it starts the moment you create it. While with a lazy version, you can create many futures without worrying, only later deciding which ones you're actually going to run.

What @qm3ster mentioned was for instance:

Just like a Future pipeline can shortcircuit on a failure, it can also conditionally never get triggered (for example because another Future going into the same liftA2 or sequence already rejected).

What I had in mind was the {maxConcurrent:X} option for Future.sequence & Future.traverse. So, you would create like 20 futures. and you would say, "run them, but I want a most 3 to run in parrallel", because you don't want to overload the server for instance. I already have this coded (minus some details which is why I didn't push yet).

Now, if you have lazy Futures, you can offer Future.sequence(futures, {maxConcurrent:3}). Because the futures were (likely, depending on how they were created) not started yet.
If you have eager futures, that overload for sequence is not possible, because the moment you created the Future, it was already started, so you've already lost. On the other hand, you can still have: Future.traverse(list, mkFuture, {maxConcurrent:3}), because traverse can create the future as it goes. So it's not like we completely give up on this feature in this case.

I guess by having lazy futures, we're giving the programmer another tool, on the other hand we give him/her one more thing to think about.. "was this future triggered? when will it be triggered?".

And there is some mess like...

myFuture.runAsync();
myFuture.onComplete(...) <-- will actually not run!! you must call runAsync() at the very end!

I mean you won't forget to call runAsync() if you're interested in the result, because you'll have to get the value out at some point, and that'll trigger it. The only risk is for side effects where it would return void. Like to hide a throbber when an action finished, inside the onComplete.

Currently leaning towards dropping the laziness, but I don't really know what's the right choice. Just guessing, I'm really on the edge. If nothing else, to be doing what most everybody else is doing and what most everybody is expecting the library to do.

[qm3ster]

myFuture.onComplete(...) <-- will actually not run!! you must call runAsync() at the very end!

Why won't this run? It should. The myFuture is either running or resolved at this point.
If it wasn't previously resolved, strictly Running, as runAsync() it should probably schedule it on setImmediate, lest we unleash zalgo, because side effects.
If it WAS previously resolved and runAsync() was thus noop, the .onComplete(...) in turn schedules the (...) on setImmediate, lest we unleash zalgo, because its sole purpose is side effects.
In both (three?) cases, ... will be executed.
nit: Assuming ... is an expression of a function and especially not an expression that throws.

[qm3ster]

I also accidentally wrote this thing:

interface Lazy<T> {
	(): T
}

interface LazyPromise<T> extends Lazy<Promise<T>> {}

import {once, memoize1, memoize2} from './memoization'

const _trigger = <T>(lp: LazyPromise<T>): Promise<T> => lp()
const trigger = memoize1(_trigger as any) as typeof _trigger

const _map = <T, U>(fn: (val: T) => U, lp: LazyPromise<T>): LazyPromise<U> =>
	once(() => trigger(lp).then(fn))
const map = memoize2(_map as any) as typeof _map

const _filter = <T>(
	predicate: (T) => boolean,
	lp: LazyPromise<T>,
): LazyPromise<T> =>
	once(() =>
		trigger(lp).then(
			val =>
				predicate(val) ? val : (Promise.reject('filtered') as Promise<T>),
		),
	)
const filter = memoize2(_filter as any) as typeof _filter

const rejectionSymbol = {}
const _sequence = <T>(lps: LazyPromise<T>[]): LazyPromise<T[]> =>
	once(() =>
		Promise.all(lps.map(lp => trigger(lp).catch(() => rejectionSymbol))).then(
			results => results.filter(r => r !== rejectionSymbol) as T[],
		),
	)
const sequence = memoize1(_sequence as any) as typeof _sequence

export const once = <R, A extends any[], F extends (...args: A) => R>(
	fn: F,
): F => {
	let real = (...args: A) => {
		const val = fn(...args)
		real = () => val
		return val
	}
	return ((...args) => real(...args)) as F
}

export const memoize1 = <R, T1 extends object, F extends (a: T1) => R>(
	fn: F,
): F => {
	const map1 = new WeakMap<T1, R>()
	return ((a: T1): R => {
		let res = map1.get(a)
		if (!res) map1.set(a, (res = fn(a)))
		return res
	}) as F
}

export const memoize2 = <
	R,
	T1 extends object,
	T2 extends object,
	F extends (a: T1, b: T2) => R
>(
	fn: F,
): F => {
	const map1 = new WeakMap<T1, WeakMap<T2, R>>()
	return ((a: T1, b: T2): R => {
		let map2 = map1.get(a)
		if (!map2) map1.set(a, (map2 = new WeakMap()))
		let res = map2.get(b)
		if (!res) map2.set(b, (res = fn(a, b)))
		return res
	}) as F
}

It's probably dumb and unperformant, with all that function wrapping instead of stateful objects, but it shows that at least some current functions can be without too much difficulty written around a Lazy of a Promise

[emmanueltouzery]

setImmediate, lest we unleash zalgo

My understanding is that we don't need to worry about that? Underlying the Future is a promise anyway, and the only thing we do is to call then on that promise, so it all goes cleanly through the event loop, it's not a sync call.

But you're right that for sanity's sake, if the future is lazy, once the future is triggered, anything called further down the line on that promise should be triggered too.

[emmanueltouzery]

I started working on a PR in which I would remove laziness and also take into account feedback on the current API from @qm3ster (who had other comments besides the laziness/eagerness), and also add the do-like constructor. I didn't strongly decide to remove laziness, but I figure also in other areas prelude.ts tried to keep things simple and working in the way people expect them to be.

I'll submit this through a PR so that people can comment on the changes. I'm still open for feedback on the laziness/eagerness aspect! Implementation simplicity is also a (lesser) concern.

[qm3ster]

If lazy futures are to exist, the triggering should only propagate up the chain, not down.

const master = Future.of(async() => 1)
const obj = {
  master
  lowBranch: master.map(x=>x-1)
  highBranch: master.map(x=>x+1)
}
Object.entries(obj).forEach(([name, future]) => {future.onSuccess(x=>{console.log(name,x)})}
obj.highBranch.runAsync()
// event loop shenanigans
// > master 1
// > highBranch 2

In the above example, > lowBranch 0 should never print.

Oh, you meant late binding.
Yeah, that's a problem.
It seems we would want the handlers to be triggered in the following case:

// 9 years later
obj.master.onSuccess(x=>{console.log('secondHandler',x)})
// almost immediately
// > secondHandler 1

But that violates the expectation of the word on from EventEmitter context, since the event has already taken place in the past.
Seems like non-triggering .then(), non-triggering .catch() and non-triggering .finally()
need names that aren't .onSuccess(), .onFailure() and .onCompletion() 😕

[emmanueltouzery]

@qm3ster yes, I think lazy futures bring lots of tricky issues, and not only for the implementor but also for the users as you highlighted, so I think we'll fall back to plain eager futures... I put out a PR if you have comments #11

[emmanueltouzery]

Closing this bug as it ended up focusing on the laziness which we discarded. I did open #12 to discuss Future.liftXXX.