-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Support async function serialization. #1311
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One correctness nit, otherwise LGTM!
let isAsync = false; | ||
if (funcString.startsWith("async ")) { | ||
isAsync = true; | ||
funcString = funcString.substr("async ".length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this runs into trouble when there's more than one space trailing an async
:
> closure.parseFunction('async function bar() {}')
[ '',
{ funcExprWithoutName: 'async function () {}',
funcExprWithName: 'async function () {}',
functionDeclarationName: undefined,
isArrowFunction: false,
capturedVariables: { required: Map {}, optional: Map {} },
usesNonLexicalThis: false } ]
> closure.parseFunction('async function bar() {}')
[ '',
{ funcExprWithoutName: 'async function /*bar*/() {}',
funcExprWithName: 'async function bar() {}',
functionDeclarationName: 'bar',
isArrowFunction: false,
capturedVariables: { required: Map {}, optional: Map {} },
usesNonLexicalThis: false } ]
The spec guarantees there won't be a newline between async
and function
so maybe we should eat all whitespace following the async
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do. we trimleft a lot here.
Matt had a good idea to just write javascript unit tests. They demonstrate the fix works :) |
// Note, i can't think of a better way to determine this. This is particularly hard because | ||
// we can't even necessary refer to async function objects here as this code is rewritten by | ||
// TS, converting all async functions to non async functions. | ||
const isAsyncFunction = func.constructor && func.constructor.name === "AsyncFunction"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we take the same approach -- throw JS along Typescript -- to get access to the AsyncFunction
constructor to test for equality?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is slightly more annoying. mostly because ts doesn't allow you to compile ts alongside js and while producing a declaration file. We could do it jsut as standalone .js file that we import and add to our makefile...
i'm not sure if i want to go to that length...
Fixes: #1306
Note: i've only manually validated this. Making a unit test for this has proven annoying. Integration test is here: pulumi/pulumi-cloud#471