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

Allow yielding a chain of then on Task #950

Open
wants to merge 2 commits into
base: v3
Choose a base branch
from

Conversation

SebastienGllmt
Copy link

@SebastienGllmt SebastienGllmt commented Dec 29, 2024

Motivation

There are cases where you need to do the following steps:

  1. Create a Task (using run)
  2. Do some computation on the Task using then
  3. yield the result of the task

example that shows how this can be used to:

class Foo {
    task: Future<number>;
    
    constructor() {
       this.task = run(() => ...).then(val => val+1);
    }

    *doWork() {
        yield* this.task;
    }
}

This was a bit tedious to do previously (see #944), and this PR simplifies this case a bit by making that calling then on a Task returns a Future (which you can yield) instead of just a Promise

Approach & Alternate Designs

You can find some background in #944 and inline comments in this PR

TODOs and Open Questions

  • Do we want to make creating Futures more easier in general? This has a chance of overlapping conceptually with the pipe concept that was recently removed, and also overlaps with the idea of call (although call returns an Operation instead of a Future)
  • Is there a way to make that then returns a Task instead of a Future? I tried this initially, but ran into some complications: Document equivalent to Promise.then #944 (comment)

Comment on lines +121 to +130
const newPromise = getHaltPromise().then(onfulfilled, onrejected);
const future: Future<NewResult> = create<Future<NewResult>>("Future", {}, {
*[Symbol.iterator]() {
return yield* call(() => newPromise);
},
then: (...args) => newPromise.then(...args),
catch: (...args) => newPromise.catch(...args),
finally: (...args) => newPromise.finally(...args),
});
return await future;
Copy link
Author

@SebastienGllmt SebastienGllmt Dec 29, 2024

Choose a reason for hiding this comment

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

note: the code here for handling the abort future is mostly the same as the then in the parent Task

This is why I think maybe it's better if we have a more general way to create a Future that is exposed somewhere that is used in both these places

There is, to some extent, a conceptual overlap between what this block of code is trying to do and what call is doing. That is to say, if call returned a Future instead of an Operation, using it here could also be an option

Comment on lines +57 to +78
/**
* Attaches callbacks for the resolution and/or rejection of the Promise.
* @param onfulfilled The callback to execute when the Promise is resolved.
* @param onrejected The callback to execute when the Promise is rejected.
* @returns A Promise for the completion of which ever callback is executed.
*/
then<TResult1 = T, TResult2 = never>(onfulfilled?: ((value: T) => TResult1 | PromiseLike<TResult1>) | undefined | null, onrejected?: ((reason: any) => TResult2 | PromiseLike<TResult2>) | undefined | null): Future<TResult1 | TResult2>;

/**
* Attaches a callback for only the rejection of the Promise.
* @param onrejected The callback to execute when the Promise is rejected.
* @returns A Promise for the completion of the callback.
*/
catch<TResult = never>(onrejected?: ((reason: any) => TResult | PromiseLike<TResult>) | undefined | null): Future<T | TResult>;

/**
* Attaches a callback that is invoked when the Promise is settled (fulfilled or rejected). The
* resolved value cannot be modified from the callback.
* @param onfinally The callback to execute when the Promise is settled (fulfilled or rejected).
* @returns A Promise for the completion of the callback.
*/
finally(onfinally?: (() => void) | undefined | null): Future<T>;
Copy link
Author

Choose a reason for hiding this comment

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

Note: these three are copy-pasted from the definition of Promise, but have the return type changed from PromiseFuture

@@ -86,10 +115,37 @@ export function createTask<T>(
});
}
},
then: (...args) => getHaltPromise().then(...args),
Copy link
Author

Choose a reason for hiding this comment

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

Note: there is one very subtle change introduced by this PR: the result of these is no longer a Promise and, instead, is now a PromiseLike. In practice, this should work everywhere (the goal of PromiseLike is that they're meant to be supported by everything magically), but it is possible to write code that only works on Promises and not PromiseLike (ex: any code that uses instanceof Promise)

@cowboyd
Copy link
Member

cowboyd commented Dec 31, 2024

I'm wondering about the corner cases here.

  1. Will the returned promises be lazy? I.e.
  2. What if the return value is an operation vs a promise vs a regular value?

I.e. what would be the expectation around this:

run(fn).then(function*(value) { return value + 1 })

@SebastienGllmt
Copy link
Author

  1. The laziness is not changed by this PR. Nothing happens (lazy) until you use await just like before this PR
  2. Currently, the behavior is the same as promises in generally. That is to say,
    1. if you return a value, it stays a value
    2. if you return a promise, it gets flattened to a value (.then(() => Promise.resolve(3)) becomes just 3)
    3. If you return a generator, it doesn't get flattened (it gets treated as case 1 and just returns a generator)

You could say that may we would like then to flatten generators as well for usability reasons, but this is not possible because the type definition of then comes from Promise (we cannot just arbitrarily change it). You could introduce a new function in the Future interface that flattens both promises in generators if we want

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.

2 participants