Emit declarations using alternative containing modules for types exported using separate export statements#56857
Conversation
…rted using separate export statements
typescript-bot
added
the
For Uncommitted Bug
src/compiler/checker.ts
Outdated
| return undefined; | ||
| } | ||
| const containers = mapDefined(candidates, candidate => getAliasForSymbolInContainer(candidate, symbol) ? candidate : undefined); | ||
| return containers.length === 1 ? getWithAlternativeContainers(containers[0]) : containers; |
There was a problem hiding this comment.
this line is ad-hoc, perhaps I should just flatMap this. From what I've seen the consumers of this return value assume some ordering - or well, operate on first items with priority or smth. So I'm not sure how it would behave in more complex scenarios with smth like: [...withAlternativeContainersA, ...withAlternativeContainersB, ...withAlternativeContainersC]
There was a problem hiding this comment.
Wait I’m confused; why would there only being a single element in the array affect whether you need to repackage it or not?
There was a problem hiding this comment.
I tried to explain this in the comment above. This is the only case I know how to reason about and about which i’m somewhat confident here. This is not the final solution - i’m seeking guidance what should be done here when there are more items in this array
There was a problem hiding this comment.
From what I've seen the consumers of this return value assume some ordering - or well, operate on first items with priority or smth
Indeed, the results returned here are intended to be in priority order for "what is best to serialize", which is, admittedly, a bit subjective. IMO, the ordering here would probably be the direct symbols for each available container symbol, followed by all their alternative symbols, in order - so not quite a flat map.
There was a problem hiding this comment.
I implemented the suggested solution. Could you take another look?
src/compiler/checker.ts
Outdated
| return undefined; | ||
| } | ||
| const containers = mapDefined(candidates, candidate => getAliasForSymbolInContainer(candidate, symbol) ? candidate : undefined); | ||
| return containers.length === 1 ? getWithAlternativeContainers(containers[0]) : containers; |
There was a problem hiding this comment.
From what I've seen the consumers of this return value assume some ordering - or well, operate on first items with priority or smth
Indeed, the results returned here are intended to be in priority order for "what is best to serialize", which is, admittedly, a bit subjective. IMO, the ordering here would probably be the direct symbols for each available container symbol, followed by all their alternative symbols, in order - so not quite a flat map.
typescript-bot
added
For Milestone Bug
Andarist
force-pushed
the
fix/dts-emit-through-separate-export-statements
branch
from
4b7ce35 to
23553bb
Compare
|
@typescript-bot test this |
|
Heya @weswigham, I've started to run the diff-based top-repos suite on this PR at 23553bb. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 23553bb. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've started to run the public perf test suite on this PR at 23553bb. You can monitor the build here. Update: The results are in! |
|
Hey @weswigham, the results of running the DT tests are ready. |
|
@weswigham Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@weswigham Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
|
Just a note to say that this change seems to have regressed our (Bloomberg's) workaround for #38111. I'll put a playground together and raise a concrete issue tomorrow 🙂 |
5 participants
fixes #56856