Resolve promise inside async generator #23887
Conversation
src/compiler/transformers/esnext.ts
Outdated
| function verb(n) { if (g[n]) i[n] = function (v) { return new Promise(function (a, b) { q.push([n, v, a, b]) > 1 || resume(n, v); }); }; } | ||
| function resume(n, v) { try { step(g[n](v)); } catch (e) { settle(q[0][3], e); } } | ||
| function step(r) { r.value instanceof __await ? Promise.resolve(r.value.v).then(fulfill, reject) : settle(q[0][2], r); } | ||
| function step(r) { |
There was a problem hiding this comment.
Per the spec, yield will always await. We already handle this behavior for yield* via __asyncDelegator. Rather than changing the helper here, we could handle this by changing the emit for the async generator body to replace yield x with yield yield __await(x).
There was a problem hiding this comment.
@rbuckton makes sense! I gave it a try and updated the PR. Could you take another look?
There was a problem hiding this comment.
yield yield __await looks to me like you're rewriting yield x into yield await x. I'm not sure those are the right semantics, even if they appear to work.
There was a problem hiding this comment.
@Kovensky, they actually are the correct semantics per the spec, specifically in 25.5.3.7 AsyncGeneratorYield:
- Set value to ? Await(value).
src/compiler/transformers/esnext.ts
Outdated
| return setOriginalNode( | ||
| setTextRange( | ||
| createYield( | ||
| createDownlevelAwait(node.expression) |
There was a problem hiding this comment.
With any transformation you need to ensure you continue visiting the subtree, so this should be createDownlevelAwait(visitNode(node.expression, visitor, isExpression))
src/compiler/transformers/esnext.ts
Outdated
| ); | ||
| } | ||
|
|
||
| if (node.expression && node.expression.kind !== SyntaxKind.AwaitExpression) { |
There was a problem hiding this comment.
The actual ECMAScript specs don't care whether you are already awaiting the operand to yield, e.g. in yield await x, you await twice: once for the await and once for the yield. It also doesn't matter if yield has an expression. Rather, yield is treated as yield undefined, and as a result we still have to await undefined before we actually yield.
It seems inefficient, but the runtime semantics would be observably different. If we don't have the same number of underling Awaits as the spec, then you could end up with observable differences in resolution timing between our downlevel emit and uplevel ECMAScript runtime behavior for the same code.
@DanielRosenwasser, @bterlson: Any thoughts?
There was a problem hiding this comment.
@agentcooper: in practice this would mean replacing line 151 with node.expression ? visitNode(node.expression, visitor, isExpression) : createVoidZero() and removing the if condition on line 146.
There was a problem hiding this comment.
@rbuckton got it. should I go ahead with the change or are we waiting for others to reply?
There was a problem hiding this comment.
Yes, go ahead and make the change.
|
Looks good, thanks for the contribution! |
3 participants
Fixes #23858, #23731.