Change export = structurally to allow other exported elements in the same file#62103
Change export = structurally to allow other exported elements in the same file#62103weswigham wants to merge 3 commits intomicrosoft:mainfrom
export = structurally to allow other exported elements in the same file#62103Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
| */ | ||
| function mergeSymbol(target: Symbol, source: Symbol, unidirectional = false): Symbol { | ||
| if (target === source) { | ||
| return source; // target and source already merged (likely same symbol found in multiple export tables) |
There was a problem hiding this comment.
This change is only needed because I reused mergeSymbolTables to build the combined symbol table in resolveStructuredTypeMembers. If it wasn't to taste, I could always write a custom table merging function that handles this case outside of the core mergeSymbol.
| } | ||
| } | ||
| } | ||
| if (mainModule.exports?.get(InternalSymbolName.ExportEquals) && moduleAugmentation.symbol.exports?.size) { |
There was a problem hiding this comment.
This is almost a carbon copy of the export * logic above - the exception being that a local declaration doesn't implicitly "beat" something added by an export = - they have equal weight and get merged.
| } | ||
| if (!symbol) { | ||
| if (isExportAssignmentAny(namespace)) { | ||
| return unknownSymbol; // no error - lookup on psuedo-`any` module |
There was a problem hiding this comment.
Previously this was handled one level up the stack because the module itself was the any-typed const. Now it's not any (and can contain other well-typed members, like @typedefs), but it contains an export= that points at any, and we check that there to reenable the error-less lookup it used to have.
There was a problem hiding this comment.
| return unknownSymbol; // no error - lookup on psuedo-`any` module | |
| return unknownSymbol; // no error - lookup on pseudo-`any` module |
src/compiler/checker.ts
Outdated
| } | ||
| getSymbolLinks(merged).cjsExportMerged = merged; | ||
| return links.cjsExportMerged = merged; | ||
| return resolveSymbol(moduleSymbol, dontResolveAlias) |
There was a problem hiding this comment.
Yeah, I could probably just delete resolveExternalModuleSymbol now - it's just an alias for resolveSymbol.
| function getExportsOfModule(moduleSymbol: Symbol): SymbolTable { | ||
| const links = getSymbolLinks(moduleSymbol); | ||
| if (!links.resolvedExports) { | ||
| links.resolvedExports = emptySymbols |
There was a problem hiding this comment.
getExportsOfModuleWorker can now recursively ask for it's own exports in cases like module.exports = exports - this short-circuits that, but still allows other things to merge into the cycle, despite the (sorta erroneous, sorta not) circular export.
| extendExportSymbols(symbols, table); | ||
| } | ||
| } | ||
| symbols.delete(InternalSymbolName.ExportEquals) |
There was a problem hiding this comment.
This maintains the "the resolved exports don't contains an export= symbol" expectation most callers have. It's naturally still accessible on the raw, unresolved symbol.exports table for callers that need it, though.
| type.constructSignatures = constructSignatures; | ||
| } | ||
|
|
||
| // And lastly, if the underlying symbol has an export=, we need to copy the type structure from it |
There was a problem hiding this comment.
This logic and the getExportsOfModuleWorker should be identical, except the former is only concerned with copying values for the sake of symbolic traversal, while this copies up all type structure from the export= target - members, signatures, and index infos.
| const isPsuedoAnyModule = (parentSymbol && isExportAssignmentAny(resolveSymbol(parentSymbol))) | ||
| const isAnyLike = isTypeAny(apparentType) || apparentType === silentNeverType || isPsuedoAnyModule; |
There was a problem hiding this comment.
| const isPsuedoAnyModule = (parentSymbol && isExportAssignmentAny(resolveSymbol(parentSymbol))) | |
| const isAnyLike = isTypeAny(apparentType) || apparentType === silentNeverType || isPsuedoAnyModule; | |
| const isPseudoAnyModule = (parentSymbol && isExportAssignmentAny(resolveSymbol(parentSymbol))) | |
| const isAnyLike = isTypeAny(apparentType) || apparentType === silentNeverType || isPseudoAnyModule; |
|
With 6.0 out as the final release vehicle for this codebase, we're closing all PRs that don't fit the merge criteria for post-6.0 patches. If you think this was a mistake and this PR fits the post-6.0 patch criteria, please post to the 6.0 iteration issue with details (specifically, which PR and which patch criteria it satisfies). Next steps for PRs:
|
4 participants
To allow a
export =to be typechecked alongside other exports in the module without the.cjsExportMergedlayering we had going on. I've also deleted the "export = can't be in the same module as other exports" error, as, well, that was the point, but if we wanted to, we could always keep that error in ts source files... but, IMO, a more limited "value exports should not be lexically before a valueexport =" is actually the only error you need to prevent runtime problems with the usual cjs/bundler emit, and would be the better error.This, in turn, should allow a cjs
module.exports =to behave identically to a TSexports =(a goal for corsa), as now the later has all the functionality of the former.I still need to go through the fourslash diffs and see what LS operations need to handle the
export=symbol handling logic getting pushed up a layer (some quickfixes, for sure), but baseline-wise, these actually mostly look really good - this change makes it so modules always have their own symbol (and are directly never forwarded to another symbol - they just behave as though they are at times), and so can always be referred to by that symbol.