Skip to content

Conversation

@karol-majewski
Copy link
Contributor

This adds IntelliSense and prevents the library consumers from passing synchronous functions.

import greenlet from 'greenlet';

/**
 * Proper async function.
 */
async function add(a: number, b: number) {
  return a + b;
}

/**
 * Synchronous function.
 */
function concat(...words: string[]) {
  return words.join('');
}

greenlet(add)(1, 2);          // OK
greenlet(concat)('oh', 'no'); // Error: Type 'string' is not assignable to type 'Promise<{}>'.

@@ -0,0 +1,3 @@
type AsyncFunction<T> = (...args: any[]) => Promise<T>;

Choose a reason for hiding this comment

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

Maybe

type AsyncFunction<T, A> = (...args: A) => Promise<T>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using (...args: A) is incorrect, because A rest parameter must be of an array type. You could write (...args: A[]), but this would lead nowhere since not all arguments have to be of the same type.

Additionally, it would break type inference and TypeScript would refuse to compile in strict mode.

Try TypeScript playground example with --strictFunctionTypes enabled.

@developit
Copy link
Owner

Should we require that the function return a Promise? Greenlet wraps invocation in a promise chain, so synchronous returns are actually supported as well.

@karol-majewski
Copy link
Contributor Author

karol-majewski commented Jan 29, 2018

@developit Good point. Unfortunately, making it work with synchronous functions is not a trivial task as of TypeScript 2.6.2 with its limited means for extracting function argument & return types.

By requiring an async function we can simply pass its signature down. For regular ones, we'd have to preserve the arguments' types, but promisify the return type.

This would require typing each overload separately, and even that solution has disadvantages: the argument's names are lost, and it's tricky to get optional parameters to work — overloads are chosen based on the arity of your function, and that is constant in our case. You'd have to provide generic types yourself, and while it may be accepted by TypeScript users, it's not possible for JavaScript developers trying to get IntelliSense. See the example here.

This might change in TypeScript 2.8, but that leaves us with the question: what should we do now?

@developit
Copy link
Owner

Wow, that's interesting haha. I guess for now let's stick to saying it's only for async functions - that's the far more compelling case, since the outward signature is unchanged when wrapping in greenlet().

Copy link
Owner

@developit developit left a comment

Choose a reason for hiding this comment

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

lets do this!

@developit developit merged commit 2e92588 into developit:master Jan 30, 2018
@karol-majewski karol-majewski deleted the typescript-definitions branch February 3, 2018 01:30
@epatpol
Copy link

epatpol commented Feb 9, 2018

When will this be available on npm? I think the latest npm version doesn't have the ts definition afaik.

@developit
Copy link
Owner

should be now.

@karol-majewski
Copy link
Contributor Author

By the way, TypeScript 2.8.3 is here, and it's easier to infer the return type of a function. We could lift the requirement of the passed function to be async.

The bad news is that the arguments' names will have to be altered, so I'm not sure if it's worth it.

See TypeScript playground.

@developit
Copy link
Owner

developit commented May 16, 2018

@karol-majewski guessing that's not backwards-compatible though? Argument names can be anything, they get compressed via Uglify anyway.

@karol-majewski
Copy link
Contributor Author

@developit It would add support for synchronous functions next to the existing asynchronous ones on the type level. This would be an extension rather than modification, thus being backward-compatible.

And I was referring to the arguments' names inferred by IntelliSense on the call site for the supplied function. ;-)

This is how it would look like.

@karol-majewski
Copy link
Contributor Author

@developit What do you think of that? ☝️

@developit
Copy link
Owner

Personally I like the idea of requiring the function be async simply in order to ensure its signature doesn't change as a result of being wrapped in greenlet.

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