-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
add fixAwaitInSyncFunction code fix #21069
Conversation
@Andy-MS Thanks, that is a lot better. I picked only the commit with the change, let me know if I should do it differently. I'm also wondering about those CRs in the tests, I saw them in some tests but not in others. Are they also an outcome of |
@@ -19,7 +19,7 @@ namespace ts.codefix { | |||
doChange(changes, context.sourceFile, getNodeToInsertBefore(diag.file, diag.start!))), | |||
}); | |||
|
|||
function getNodeToInsertBefore(sourceFile: SourceFile, pos: number): Node | undefined {//name | |||
function getNodeToInsertBefore(sourceFile: SourceFile, pos: number): Node | undefined {// name |
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.
Whoops, I should have removed the comment before pushing.
We could also consider changing an explicit return type |
src/compiler/diagnosticMessages.json
Outdated
@@ -3843,6 +3843,10 @@ | |||
"category": "Message", | |||
"code": 90028 | |||
}, | |||
"Convert to 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.
nit. Add async modifier to containing function
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.
@DanielRosenwasser any better suggestions for the message?
if (isVariableDeclaration(containingFunction.parent) && | ||
containingFunction.parent.type && | ||
isFunctionTypeNode(containingFunction.parent.type)) { | ||
return containingFunction.parent.type.type; |
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.
Also check for a .type
immediately on either of these, i.e. function(): number { ... }
or (): number => { ... }
.
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.
After adding this change I refactored getReturnType
to not use a switch statement at all since with the added condition the two if statements handle all cases.
function doChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, insertBefore: Node, returnType: TypeNode | undefined): void { | ||
if (returnType) { | ||
const entityName = getEntityNameFromTypeNode(returnType); | ||
if (!entityName || entityName.getText() !== "Promise") { |
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.
if (!entityName || entityName.kind !== SyntaxKind.Identifier || entityName.text !== "Promise")
getAllCodeActions: context => codeFixAll(context, errorCodes, (changes, diag) => | ||
doChange(changes, context.sourceFile, getNodeToInsertBefore(diag.file, diag.start!))), | ||
getAllCodeActions: context => codeFixAll(context, errorCodes, (changes, diag) => { | ||
const token = getTokenAtPosition(diag.file, diag.start!, /*includeJsDocComment*/ false); |
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.
Use a function getNodes(...): { insertBefore: Node, returnType: Type | undefined } | undefined
to avoid repeating this. That would also let you combine the two switch statements from getNodeToInsertBefore
and getReturnTypeNode
.
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.
(but note @mhegazy's comment about the message)
Fixes #21034
This is pretty straightforward, replacing function expressions/declarations, method declarations and arrow functions with corresponding ones with the async modifier.
There are 2 issues that I need some help with:
1. Replacing the entire node seems to omit the line break at the end of the replaced node. So that this:
Becomes:
This happens both in the unit tests, and in manual e2e testing with vscode.
2.
codeFixAwaitInSyncFunction7
, the test case of anfor-await-of
loop, fails -I tried to debug this for a while, but nothing conclusive. This does not happen when I use the compiled tsserver.js in vscode to test the code fix.