Missing import code fix - include export assignment properties when looking for module exports #17376
Conversation
| if (symbolTable) { | ||
| let symbol = symbolTable.get(memberName); | ||
| if (!symbol) { | ||
| const exportEquals = resolveExternalModuleSymbol(moduleSymbol); |
There was a problem hiding this comment.
The export properties don't obviously depend on symbolTable. Is there a reason they're not checked if symbolTable is undefined? It looks like getExportsAndPropertiesOfModule, above, doesn't have this restriction, but it may simply be missing an optimization.
There was a problem hiding this comment.
It also seems like the first attempt (i.e. the exports) could be attempted by calling tryGetMemberInModuleExports.
There was a problem hiding this comment.
Alternatively, we could merge the APIs and pass a flag.
There was a problem hiding this comment.
At this point, I'll keep the functions separate in the interest of keeping the public API stable. I am changing the code to have this one call tryGetMemberInModuleExports
|
|
||
| tryGetMemberInModuleExports(memberName: string, moduleSymbol: Symbol): Symbol | undefined; | ||
| /** Unlike `tryGetMemberInModuleExports`, this includes properties of an `export =` value. */ | ||
| /* @internal */ tryGetMemberInModuleExportsAndProperties(memberName: string, moduleSymbol: Symbol): Symbol | undefined; |
There was a problem hiding this comment.
I feel that this goes hand in hand with getExportsAndPropertiesOfModule and that one's internal
|
|
||
| // check exports with the same name | ||
| const exportSymbolWithIdenticalName = checker.tryGetMemberInModuleExports(name, moduleSymbol); | ||
| const exportSymbolWithIdenticalName = checker.tryGetMemberInModuleExportsAndProperties(name, moduleSymbol); |
There was a problem hiding this comment.
Should this check be skipped if name is "default"?
There was a problem hiding this comment.
As discussed, I don't think name can legally be "default". Will add a comment to clarify.
|
|
||
| // @Filename: foo.ts | ||
| //// interface MyStatic { | ||
| //// bar(): void; |
There was a problem hiding this comment.
If I'm reading the code correctly, it seems like the case that distinguishes the two tryGetMemberInModuleExports* call-sites is a property named "default". Would it be interesting to test that explicitly.
amcasey
left a comment
There was a problem hiding this comment.
Seems sensible but I don't understand clearly enough to "Approve".
amcasey
left a comment
There was a problem hiding this comment.
Thanks for clarifying offline!
| } | ||
| } | ||
|
|
||
| Debug.assert(name !== "default"); |
There was a problem hiding this comment.
Can you please add a comment explaining why this is true? Something like "default is a keyword and is not valid as an expression"?
Fixes #13377