Skip to content

Async code fix issues concerning underscores and nested promises #27156

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

Merged
merged 12 commits into from
Sep 18, 2018

Conversation

uniqueiniquity
Copy link
Contributor

Fixes #27112.

If the handler function for a promise itself returns a promise, then the value of the outer promise is resolved to the value of the inner. However, currently the async code fix does not wrap await the inner promise in this situation. This PR changes the behavior so that if the handler of a promise that's being fixed returns a promise, an await is added.

Additionally, this PR fixes an issue where if a promise handler doesn't have a bound parameter name (e.g., _), then the preceding promise handler would be deleted. This caused issues when the previous promise handler had side effects.

@uniqueiniquity uniqueiniquity added this to the TypeScript 3.1 milestone Sep 17, 2018
@uniqueiniquity uniqueiniquity requested review from amcasey and a user September 17, 2018 19:38
@uniqueiniquity uniqueiniquity changed the title Promises and underscores Async code fix issues concerning underscores and nested promises Sep 17, 2018
@amcasey
Copy link
Member

amcasey commented Sep 17, 2018

Would it be interesting to test the case where the handler returns a promise of a promise?

@@ -391,9 +389,14 @@ namespace ts.codefix {
return [createReturn(getSynthesizedDeepClone(node))];
}

function createVariableDeclarationOrAssignment(prevArgName: SynthIdentifier, rightHandSide: Expression, transformer: Transformer): NodeArray<Statement> {
function createVariableDeclarationOrAssignment(prevArgName: SynthIdentifier | undefined, rightHandSide: Expression, transformer: Transformer): NodeArray<Statement> {
if (!prevArgName || prevArgName.identifier.text.length === 0) {
Copy link

Choose a reason for hiding this comment

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

When does the second condition here ever happen? Shouldn't we just avoid synthesizing an empty identifier in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. There's a place in the code here where we explicitly do that, but I can find another way to handle that case.

@uniqueiniquity uniqueiniquity merged commit e40ce24 into microsoft:master Sep 18, 2018
@uniqueiniquity uniqueiniquity deleted the promisesAndUnderscores branch October 5, 2018 16:08
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.

in async code fix, handle promise handler callbacks that return a promise
2 participants