In JS declaration emit, move imports painted in nested contexts to the root private context #39818
Conversation
…e root private context
typescript-bot
added
Author: Team
For Milestone Bug
sandersn
left a comment
sandersn
left a comment
There was a problem hiding this comment.
One possible bug and one request for a test.
| deferredPrivates.set(getSymbolId(symbol), symbol); | ||
| // Blanket moving (import) aliases into the root private context should work, since imports are not valid within namespaces | ||
| // (so they must have been in the root to begin with if they were real imports) cjs `require` aliases (an upcoming feature) | ||
| // will throw a wrench in this, since those may have been nested, but we'll need to synthesize them in the outer scope |
There was a problem hiding this comment.
can you add a test for the commonjs case so I'll be forced to fix it before merging that PR?
There was a problem hiding this comment.
I mean, a copy of jsDeclarationsImportAliasExposedWithinNamespace.ts but with cjs requires instead of import and module.exports instead of export should do; i'll add it, but you'll still need to watch it (since it may do... nothing).
There was a problem hiding this comment.
I've added jsDeclarationsImportAliasExposedWithinNamespaceCjs.ts - it currently has errors and unexpected output (due to the nested typedefs not resolving correctly with cjs exports).
| const isExternalImportAlias = !!(symbol.flags & SymbolFlags.Alias) && !some(symbol.declarations, d => | ||
| !!findAncestor(d, isExportDeclaration) || | ||
| isNamespaceExport(d) || | ||
| (isImportEqualsDeclaration(d) && !isExternalModuleReference(d.moduleReference)) |
There was a problem hiding this comment.
this last line looks for import x = y, not import x = require('y'), right? I didn't think these declarations needed to be moved, so that seems backwards. But maybe I'm reading it backward.
There was a problem hiding this comment.
Yeah, but that's correct; the logic goes "An alias is an external import alias if it is not an import to any of an export (or any kind) or an alias to a local reference."
| deferredPrivates.set(getSymbolId(symbol), symbol); | ||
| // Blanket moving (import) aliases into the root private context should work, since imports are not valid within namespaces | ||
| // (so they must have been in the root to begin with if they were real imports) cjs `require` aliases (an upcoming feature) | ||
| // will throw a wrench in this, since those may have been nested, but we'll need to synthesize them in the outer scope |
3 participants
Fixes #37552