-
Notifications
You must be signed in to change notification settings - Fork 12.8k
CommonJS imports support destructuring+property access #40702
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
CommonJS imports support destructuring+property access #40702
Conversation
Fixes #40578 for prettier
src/compiler/checker.ts
Outdated
// if symbolFromVariable is export - get its final target | ||
symbolFromVariable = resolveSymbol(symbolFromVariable, dontResolveAlias); | ||
let symbolFromModule = getExportOfModule(targetSymbol, specifier, dontResolveAlias); | ||
|
||
const commonJSPropertyAccess = getCommonJSPropertyAccess(node); |
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.
Shouldn't all this work be under getTargetOfAliasDeclaration
(which resolveSymbol
eventually calls out to), and not in getExternalModuleMember
directly?
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.
in the test case below, resolveSymbol is called first on the BindingElement a
, which goes to getTargetOfImportSpecifier. Do you mean that function? I put the code in getExternalModuleMember because it seemed similar to the export=
code above, and because getExternalModuleMember
is called several places.
But looking again, it's only two other places, both exports where this wouldn't apply.
markSymbolOfAliasDeclarationIfTypeOnly(node, /*immediateTarget*/ undefined, resolved, /*overwriteEmpty*/ false); | ||
return resolved; | ||
} | ||
|
||
function getCommonJSPropertyAccess(node: Node) { | ||
if(isVariableDeclaration(node) && node.initializer && isPropertyAccessExpression(node.initializer)) { |
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(isVariableDeclaration(node) && node.initializer && isPropertyAccessExpression(node.initializer)) { | |
if (isVariableDeclaration(node) && node.initializer && isPropertyAccessExpression(node.initializer)) { | |
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.
I tried to commit your suggestion directly but github kept claiming the PR was recently updated =(
Thanks to @weswigham for noticing this. Somehow it passed the linter.
Fun fact: This is also what Node recommends for using the |
CommonJS imports now support destructuring a require that is part of a property access expression:
Previously either worked alone but not together:
Fixes #40578 for prettier; I didn't think anybody did this, so I let the tests fail earlier. Turns out prettier does!