Skip to content

Emit declarations using alternative containing modules for types exported using separate export statements - #56857

Merged
Wesley Wigham (weswigham) merged 2 commits into
microsoft:mainfrom
Andarist:fix/dts-emit-through-separate-export-statements
Jan 9, 2024
Merged

Emit declarations using alternative containing modules for types exported using separate export statements#56857
Wesley Wigham (weswigham) merged 2 commits into
microsoft:mainfrom
Andarist:fix/dts-emit-through-separate-export-statements

Conversation

@Andarist

Copy link
Copy Markdown
Contributor

fixes#56856

@typescript-botTypeScript Bot (typescript-bot) added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Dec 23, 2023
Comment threadsrc/compiler/checker.ts Outdated
return undefined;
}
const containers = mapDefined(candidates, candidate => getAliasForSymbolInContainer(candidate, symbol) ? candidate : undefined);
return containers.length === 1 ? getWithAlternativeContainers(containers[0]) : containers;

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait I’m confused; why would there only being a single element in the array affect whether you need to repackage it or not?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I implemented the suggested solution. Could you take another look?

Comment threadsrc/compiler/checker.ts Outdated
return undefined;
}
const containers = mapDefined(candidates, candidate => getAliasForSymbolInContainer(candidate, symbol) ? candidate : undefined);
return containers.length === 1 ? getWithAlternativeContainers(containers[0]) : containers;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Andarist
Mateusz Burzyński (Andarist)force-pushed the fix/dts-emit-through-separate-export-statements branch from 4b7ce35 to 23553bbCompareJanuary 6, 2024 08:58
@weswigham

Copy link
Copy Markdown
Member

@typescript-bot

TypeScript Bot (typescript-bot) commented Jan 9, 2024

Copy link
Copy Markdown
Contributor

Heya Wesley Wigham (@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!

@typescript-bot

TypeScript Bot (typescript-bot) commented Jan 9, 2024

Copy link
Copy Markdown
Contributor

Heya Wesley Wigham (@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!

@typescript-bot

TypeScript Bot (typescript-bot) commented Jan 9, 2024

Copy link
Copy Markdown
Contributor

Heya Wesley Wigham (@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!

@typescript-bot

Copy link
Copy Markdown
Contributor

Hey Wesley Wigham (@weswigham), the results of running the DT tests are ready.
Everything looks the same!
You can check the log here.

@typescript-bot

Copy link
Copy Markdown
Contributor

Wesley Wigham (@weswigham)
The results of the perf run you requested are in!

Here they are:

tsc

Comparison Report - baseline..pr
MetricbaselineprDeltaBestWorstp-value
self-build-src - node (v20.5.1, x64)
Memory used2,739,265k (± 5.68%)2,656,870k (± 5.35%)~2,574,037k2,921,340kp=0.230 n=6
Parse Time5.01s (± 0.85%)5.01s (± 0.43%)~4.99s5.04sp=0.748 n=6
Bind Time1.97s (± 1.09%)1.99s (± 1.04%)~1.96s2.02sp=0.517 n=6
Check Time32.02s (± 0.31%)32.02s (± 0.55%)~31.72s32.26sp=0.936 n=6
Emit Time2.80s (± 2.69%)2.82s (± 3.69%)~2.68s2.99sp=0.936 n=6
Total Time41.83s (± 0.21%)41.86s (± 0.16%)~41.76s41.94sp=0.471 n=6
self-compiler - node (v20.5.1, x64)
Memory used418,813k (± 0.02%)418,829k (± 0.02%)~418,755k418,981kp=0.377 n=6
Parse Time2.89s (± 0.75%)2.90s (± 0.68%)~2.87s2.92sp=0.871 n=6
Bind Time1.13s (± 0.48%)1.13s (± 0.48%)~1.13s1.14sp=1.000 n=6
Check Time14.09s (± 0.22%)14.04s (± 0.40%)~13.96s14.11sp=0.077 n=6
Emit Time1.04s (± 0.78%)1.04s (± 0.52%)~1.04s1.05sp=0.441 n=6
Total Time19.17s (± 0.26%)19.12s (± 0.24%)~19.04s19.16sp=0.171 n=6
vscode - node (v20.5.1, x64)
Memory used2,827,995k (± 0.00%)2,828,008k (± 0.00%)~2,827,977k2,828,096kp=0.689 n=6
Parse Time10.73s (± 0.41%)10.73s (± 0.17%)~10.71s10.76sp=0.936 n=6
Bind Time3.42s (± 0.47%)3.43s (± 0.40%)~3.41s3.44sp=0.514 n=6
Check Time56.17s (± 0.15%)56.16s (± 0.24%)~55.97s56.36sp=0.872 n=6
Emit Time16.20s (± 0.96%)16.27s (± 0.46%)~16.17s16.36sp=0.261 n=6
Total Time86.53s (± 0.19%)86.59s (± 0.18%)~86.34s86.77sp=0.470 n=6
System info unknown
Hosts
  • node (v20.5.1, x64)
Scenarios
  • self-build-src - node (v20.5.1, x64)
  • self-compiler - node (v20.5.1, x64)
  • vscode - node (v20.5.1, x64)
BenchmarkNameIterations
Currentpr6
Baselinebaseline6

Developer Information:

Download Benchmarks

@typescript-bot

Copy link
Copy Markdown
Contributor

Wesley Wigham (@weswigham) Here are the results of running the top-repos suite comparing main and refs/pull/56857/merge:

Something interesting changed - please have a look.

Details

chakra-ui/chakra-ui

4 of 28 projects failed to build with the old tsc and were ignored

packages/components/tsconfig.build.json

  • error TS5056: Cannot write file '/mnt/ts_downloads/chakra-ui/packages/components/dist/types/menu/menu.stories.d.ts' because it would be overwritten by multiple input files.
    • Project Scope

@weswigham
Wesley Wigham (weswigham) merged commit 4557e34 into microsoft:mainJan 9, 2024
@acutmore

Copy link
Copy Markdown
Contributor

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 🙂

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

For Milestone BugPRs that fix a bug with a specific milestone

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Declarations can't be emitted when types from inner modules are exported through separate export declarations

6 participants

@Andarist@weswigham@typescript-bot@acutmore@fatcerberus@sandersn