Skip to content

Support binding patterns in Promise -> async/await refactor#31008

Merged
andrewbranch merged 11 commits into
microsoft:masterfrom
andrewbranch:bug/29358
Apr 24, 2019
Merged

Support binding patterns in Promise -> async/await refactor#31008
andrewbranch merged 11 commits into
microsoft:masterfrom
andrewbranch:bug/29358

Conversation

@andrewbranch
Copy link
Copy Markdown
Member

@andrewbranch andrewbranch commented Apr 18, 2019

Fixes #29358—the original refactor unsafely cast the params in callbacks in .then() and .catch() from BindingName to Identifier, which meant if they were in fact a binding pattern, the refactor would crash.

Comment thread src/services/utilities.ts
const transformationBody = getTransformationBody(func, prevArgName, argName, node, transformer);
const catchArg = argName ? argName.identifier.text : "e";
const catchClause = createCatchClause(catchArg, createBlock(transformationBody));
const catchArg = argName ? "identifier" in argName ? argName.identifier.text : argName.bindingPattern : "e";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FWIW, there was at least the intent at one point to forbid using the in operator (see the tslint.json in the root). It just so happens that it's a custom rule that doesn't actually work, so that's why this didn't raise an error. I'm not sure if that's something we still don't want to use or not.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

👍🏽 Done, switched to using a const enum discriminated union and a helper function for discriminating between them.

Comment thread src/services/utilities.ts
return transformThen(node, transformer, outermostParent, prevArgName);
}
else if (isCallExpression(node) && hasPropertyAccessExpressionWithName(node, "catch") && nodeType && !!transformer.checker.getPromisedTypeOfPromise(nodeType)) {
else if (isCallExpression(node) && hasPropertyAccessExpressionWithName(node, "catch") && nodeType && !!transformer.checker.getPromisedTypeOfPromise(nodeType) && (!prevArgName || "identifier" in prevArgName)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a particular reason we fail if we see a catch but the prevArgName is a binding element?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It doesn't actually fail, it falls through to

else if (nodeType && transformer.checker.getPromisedTypeOfPromise(nodeType)) {
  return transformPromiseCall(node, transformer, prevArgName);
}

and basically becomes a no-op. This happens only in some really specific structures where a then and a catch both return a value and then another then destructures that value. The result is that the outer promise gets transformed and the inner, nested one doesn't. (And really, the resulting code is more concise than the fully transformed async version in the examples I've been able to track down.) But strictly speaking, this is an incomplete solution. Let me give it another hour or two and see if I can come up with something better.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Got it! e7fb18e now handles this case, and you can see the corresponding test case here

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like we're missing the baseline file for it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hm, so we are. Updated.

Comment thread src/services/codefixes/convertToAsyncFunction.ts Outdated
@andrewbranch andrewbranch merged commit 6608349 into microsoft:master Apr 24, 2019
@andrewbranch andrewbranch deleted the bug/29358 branch April 24, 2019 15:43
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

getCodeFixes — Cannot read property 'length' of undefined

4 participants