Add missing arguments to typeToTypeNode. #38336
Conversation
| if (!returnTypeNode) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const returnType = checker.getTypeFromTypeNode(returnTypeNode); | ||
| const promisedType = checker.getAwaitedType(returnType) || checker.getVoidType(); | ||
| const promisedTypeNode = checker.typeToTypeNode(promisedType); | ||
| const promisedTypeNode = checker.typeToTypeNode(promisedType, /*enclosingDeclaration*/ func, /*flags*/ undefined); |
There was a problem hiding this comment.
It looks like CFA can't tell func is not undefined here?
There was a problem hiding this comment.
That's right - we can tell that returnTypeNode is defined but we don't tie that info back.
There was a problem hiding this comment.
Technically if we removed the check though, we'd be in a bad situation.
There was a problem hiding this comment.
I was just observing that there seemed to be a potential improvement that could be made to CFA, not suggesting any changes to this code.
|
So, important, after-the-fact question: Why does this quick-fix need to serialize the type at all (in most cases, at least)? Couldn't it just construct a type reference to promise around (a clone of) the existing type node, without re-serializing it? Eg, |
|
@weswigham I could be misreading either your comment or the code, but I think it's removing |
|
Ah yeah. Even better in that case, yeah, we could just extract the type parameter from the reference and reuse it any time that node's type matches the awaited type. This way we'd preserve any aliases/idiosyncrasies the user used, whenever possible. |
|
I thought about this, but the PR is already in. We'd want to see if the awaited type is the same as the original type and if so just wrap it in |
Fixes #38335.