Improve test for 'symbol.exports' #25523
Conversation
ghost
requested a review
from 
| @@ -171,8 +171,8 @@ namespace ts { | |||
| } | |||
| const t = getTypeOfSymbol(symbol); | |||
| visitType(t); // Should handle members on classes and such | |||
| if (symbol.flags & SymbolFlags.HasExports) { | |||
| symbol.exports!.forEach(visitSymbol); | |||
| if (symbol.exports) { | |||
There was a problem hiding this comment.
so when does it have the flag but no exports? this is more concerning than the error really.
There was a problem hiding this comment.
We don't have a repro for this unfortunately.
There was a problem hiding this comment.
we only use HasExports in two places other than this one.. once in the binder and once
in the checker.. I wounder if we can just remove it all together, and check exports instead.
There was a problem hiding this comment.
@weswigham and @sandersn thoughts?
There was a problem hiding this comment.
HasExports afaik is set on container symbols which can have exports, while the member is present if there actually are any. I imagine the usage in the checker is likely incorrect - AFAIK it should only be used in the binder to determine how to initialize a symbol.
There was a problem hiding this comment.
It's only an alias, so even in the binder, the code might actually be clearer without it. (or with a comment instead)
|
@sandersn A JS baseline needed an update -- pretty sure this is correct since |
|
That baseline came from #22461, which has examples from graceful-fs, uglify and webpack. All of those assign exports to |
| @@ -225,11 +225,11 @@ namespace ts { | |||
| node.symbol = symbol; | |||
| symbol.declarations = append(symbol.declarations, node); | |||
|
|
|||
| if (symbolFlags & SymbolFlags.HasExports && !symbol.exports) { | |||
| if (symbolFlags & (SymbolFlags.Class | SymbolFlags.Enum | SymbolFlags.Module | SymbolFlags.Variable) && !symbol.exports) { | |||
Fixes #25484