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

feat: call accepts plain functions #832

Merged
merged 1 commit into from
Nov 30, 2023
Merged

feat: call accepts plain functions #832

merged 1 commit into from
Nov 30, 2023

Conversation

neurosnap
Copy link
Collaborator

@neurosnap neurosnap commented Nov 16, 2023

Motivation

As library developers, we might want to require the user to understand delimited continuations or any of the constructs created by effection. Sometimes we want to let the end-user provide anything reasonable we call() knows how to reflect what was passed into the function and it will handle it automatically. The most obvious example is Promise. We don't have to require the user to convert a Promise into an Operation.

Being able to pass a vanilla function into call() would also be nice since it is trivial for us to detect and accept.

So in summary, we want to support the following variables to call():

call(() => thing);
call(async () => thing);
call(function*() {});

Approach

We try to inspect the type of the variables passed into call() and figure out how to evaluate it.

@cowboyd cowboyd self-requested a review November 20, 2023 20:33
@neurosnap neurosnap force-pushed the v3-callable branch 2 times, most recently from ca590f7 to b219cd0 Compare November 21, 2023 21:34
@neurosnap neurosnap marked this pull request as ready for review November 21, 2023 21:35
lib/call.ts Outdated
export function call<T>(callable: () => Operation<T>): Operation<T>;
export function call<T>(callable: () => Promise<T>): Operation<T>;
export function call<T>(callable: () => T): Operation<T>;
export function call<T>(callable: Callable<T>): Operation<T | Iterable<T>> {
Copy link
Member

Choose a reason for hiding this comment

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

Why might this return an Operation<Iterable>? If you have call(() => []) which override will it select? The one on line 68 or 69?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right, removed. Probably old code from when I was experimenting.

Copy link
Member

@cowboyd cowboyd left a comment

Choose a reason for hiding this comment

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

I think this is the best option we have. As noted, there is a small discrepancy in the types and the runtime behavior in the event that you call(() => instructions) where instructions is an array of Instruction.

This is a very tiny edge case.

The only remaining decision is whether to support bare values. In other words, should callable be:

| (() => T))
| (() => Promise<T>))
| (() => Operation<T>))

or

| T
| Operation<T>
| Promise<T>
| (() => T))
| (() => Promise<T>))
| (() => Operation<T>))

It feels like we should support one or the other, but not somewhere in between.

lib/call.ts Outdated
Comment on lines 8 to 11
function isPromise(p: unknown): boolean {
if (!p) return false;
return isFunc((p as PromiseLike<unknown>).then);
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we use guards where possible? It saves a little casting when you use them.

Suggested change
function isPromise(p: unknown): boolean {
if (!p) return false;
return isFunc((p as PromiseLike<unknown>).then);
}
function isPromise<T>(p: unknown): p is Promise<T> {
if (!p) return false;
return isFunc((p as Promise<T>).then);
}

lib/channel.ts Outdated
Comment on lines 59 to 60
send: (p) => call(() => signal.send(p)),
close: (p) => call(() => signal.close(p)),
Copy link
Member

Choose a reason for hiding this comment

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

Since we can't get rid of lift, let's continue to use it here. We can revisit the best way to do this later, but lift is not going to create a frame which is preferable.

@neurosnap neurosnap force-pushed the v3-callable branch 2 times, most recently from 7db0609 to b5f6297 Compare November 29, 2023 17:33
@neurosnap neurosnap requested a review from cowboyd November 29, 2023 17:34
})).resolves.toEqual(eq);
});

it("evaluates a string iterator", async () => {
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't call this a string iterator, but just a function returning a string.

lib/call.ts Outdated
| (() => Operation<T>)
| (() => Promise<T>);
| (() => Iterator<T>)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be Iterator<Instruction, T>?

lib/call.ts Outdated
Comment on lines 5 to 23
function isFunc(f: unknown): boolean {
return typeof f === "function";
}
function isPromise<T>(p: unknown): p is Promise<T> {
if (!p) return false;
return isFunc((p as Promise<T>).then);
}
// iterator must implement both `.next` and `.throw`
// built-in iterators are not considered iterators to `call()`
function isInstructionIterator<T>(it: unknown): it is Iterator<Instruction, T> {
if (!it) return false;
return isFunc((it as Iterator<Instruction, T>).next) &&
isFunc((it as Iterator<Instruction, T>).throw);
}
function isIterable<T>(it: unknown): it is Iterable<T> {
if (!it) return false;
return typeof (it as Iterable<T>)[Symbol.iterator] === "function";
}

Copy link
Member

Choose a reason for hiding this comment

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

Can we move these private functions to the bottom of the file and lead with the implementation of call()?

lib/call.ts Outdated
Comment on lines 25 to 26
* Pause the current operation, and run a promises, async functions, or
* operations within a new scope. The calling operation will resumed (or errored)
Copy link
Member

Choose a reason for hiding this comment

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

Looks like there's a slight grammatical error here. Might as well fix this now.

Suggested change
* Pause the current operation, and run a promises, async functions, or
* operations within a new scope. The calling operation will resumed (or errored)
* Pause the current operation, and run a promise, async function, or
* operation within a new scope. The calling operation will resumed (or errored)

Copy link
Member

@cowboyd cowboyd left a comment

Choose a reason for hiding this comment

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

Looks good!

test/call.test.ts Outdated Show resolved Hide resolved
We also export `Callable` so it is available in userland.
@neurosnap neurosnap merged commit 55d40fa into v3 Nov 30, 2023
1 check passed
@neurosnap neurosnap deleted the v3-callable branch November 30, 2023 15:52
@neurosnap neurosnap changed the title feat: Callable feat: call accepts plain functions Nov 30, 2023
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