React HOCs generate extremely large and incorrect .d.ts declaration files, regression from 4.1.3

[ms]

Bug Report

While trying to upgrade from 4.1.3 to 4.2.3 in our monorepo, we ran into a regression that's preventing us from upgrading at the moment. (This is the same monorepo mentioned in #41570 cc @brieb @lencioni)

When using React HOCs and letting TS infer returned types, the type inferred and included in the corresponding .d.ts is:

• much larger, with a large number of conditional types
• incorrect, in some instances, removing wrapped component props optionality
• inconsistent with direct type checking (leading to the language server reporting no errors but tsc reporting errors, in some cases)
• much slower during compilation

🔎 Search Terms

HOC

Declaration mismatch

Upgrade

Performance regression

🕗 Version & Regression Information

4.2.* seems to exhibit the issue. A git bisect with locally built compilers seems to point to 34f0e32 as the first bad commit (however, despite the title of the commit, the npm package for 4.2.1 also has the issue).

⏯ Playground Link

Playground link with relevant code

Note that your browser may slow down significantly. Check the .d.ts tab. Compare with version 4.1.3.

This does not fully reproduce the error because of cross project dependencies (the type is not only large but also incorrect).

💻 Code

To reproduce the issue in more details, check https://github.com/ms/ts-hoc-repro (to reproduce: npm i && npx tsc -p projectA/ && npx tsc -p projectB/). The two issues:

• projectA/withSomething.d.ts is very large, as seen on the playground, see https://github.com/ms/ts-hoc-repro/blob/master/projectA/withSomething.d.ts
• projectB/ won't type check because it complains about a missing prop, but that prop is optional in the inner component (so the HOC return type is incorrect)

🙁 Actual behavior

• Incorrect type for wrapped components
• Large .d.ts
• Slow compilation using composite: true because of the large .d.ts

🙂 Expected behavior

• Correct type for wrapped component
• .d.ts files equivalent to 4.1.3
• No speed regression on compilation time

Last, I ran tsc with --generateTrace for 4.1 and 4.2. Here's a screenshot for 4.2 with transformNodes, in the emit phase, taking multiple seconds (this call is invisible for 4.1)

Workaround

We've been able to mitigate this issue by explicitly annotating the return type of the HOC (using the exact same generic helpers, ElementConfig, etc.)

[andrewbranch]

@amcasey have you seen this come up in any of your recent perf investigations?

[amcasey]

@andrewbranch I was only looking at compile time and not .d.ts size or correctness, but I wouldn't be too surprised to learn this is related to #43485 or #43422. I believe both have fixes in PR (#43437 too), so it might be interesting to make a temporary branch combining the fixes to see if it helps. You'll probably want to toss in your missing-await fix too to make upgrading easier.

[amcasey]

That's a crazy amount of emit time. I'm aware of a 5% regression, but nothing like that. I'm looking at an internal codebase with similar looking traces, but they didn't mention a regression compared to previous TS versions.

[amcasey]

@ms can you give us a sense of how much is being emitted? e.g. MB of .js / .t.ds / .tsbuildinfo?

[ms]

In the screenshot, the emit time corresponds to the repo I linked to (so just 1 project with an HOC and 1 project with a function component wrapped by that HOC, almost no code). Here is the repo: https://github.com/ms/ts-hoc-repro (to repro, npm i && npx tsc -p projectA/ && npx tsc -p projectB/)

The emit time for our real projects represent a smaller proportion of the total compile time (the most I've seen was 20% or so), but that's most likely because type checking is more complex so it takes a larger share of the total. In our monorepo as a whole, we're seeing a regression of approximately 10%, across about 1000 projects. A few specific projects with HOCs are taking 100% more time than 4.1.

The total .tsbuild size for 4.1 for the entire monorepo is 1183400 .tsbuild/ (1.2G)
The total .tsbuild size for 4.2 for the entire monorepo is 1215896 .tsbuild/ (1.2G as well, not a huge difference)

I'd like to highlight for you this small part of the declaration file (which repeats multiple time in the file): https://github.com/ms/ts-hoc-repro/blob/508b7801655105023280eb3ae6de19bddbf8323b/projectA/withSomething.d.ts#L21-L30 as you can see, it seems to be inlining the types but some of the result seems very weird. In particular there are a bunch of ElementProps<C> extend any ? A : B which I think are equivalent to A and a bunch of ElementProps<C> extends never ? A : B which I think are equivalent to B. It almost looks like 4.2 is trying to unroll a recursive type.

[DanielRosenwasser]

Just as a quick check, are you seeing similar results with our nightly builds?

Sounds like @andrewbranch already tried using the nightly on the repro.

[andrewbranch]

None of the PRs @amcasey pointed out seem to have an impact on this one.

[andrewbranch]

@weswigham, can you check out the change in declaration emit on this playground between 4.1 and 4.2 and see if you recognize an obvious culprit?

[ms]

With a bit more bisecting, I believe the problematic commit is c456bbd (cc @brieb)

[ms]

With the huge caveat that I do not understand this code, this diff "fixes" the issue in the commit I mentioned above:

diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
index 5d293f3297..27947bba5e 100644
--- a/src/compiler/checker.ts
+++ b/src/compiler/checker.ts
@@ -12447,6 +12447,7 @@ namespace ts {
         }
 
         function getTypeAliasInstantiation(symbol: Symbol, typeArguments: readonly Type[] | undefined, aliasSymbol?: Symbol, aliasTypeArguments?: readonly Type[]): Type {
+            aliasSymbol = undefined;
             const type = getDeclaredTypeOfSymbol(symbol);
             if (type === intrinsicMarkerType && intrinsicTypeKinds.has(symbol.escapedName as string) && typeArguments && typeArguments.length === 1) {
                 return getStringMappingType(symbol, typeArguments[0]);

[amcasey]

FYI @ahejlsberg, it looks like #42284 may have caused a perf regression in emit. Repro steps are here.

[ms]

it looks like #42284 may have caused a perf regression in emit

Thanks for looking into it 🙏 I want to mention, in case it got lost in the discussion, that it also seems to yield incorrect results whereby the .d.ts type doesn't match the source type in some subtle ways (default props not being optional, in the repro). That hasn't been a blocker for us because annotating the return value of the HOC explicitly solves the problem, but it's probably worth investigating as well.

[ahejlsberg]

This is indeed caused by #42284 and is a result of us associating new type aliases with types where we previously didn't. Specifically, the withSomething function declares the following two local types:

type ComponentProps = ElementConfig<C>;
type Props = Omit<ComponentProps, keyof WithSomethingProps>;

Previously we would not associate type aliases with the resulting types. In other words, we'd print back any reference to ComponentProps as ElementConfig<C> and ElementConfig<C> would remain the alias associated with the type. We now associate the alias ComponentProps with the type and print that back. That's all great, except in cases where the alias isn't reachable--which is the case here because local type aliases declared within a function aren't reachable from outside that function. So, with no reachable alias, we resort to structurally inlining the type, and that obviously doesn't go well.

The quick workaround is to declare the local types as follows:

type ComponentProps = [ElementConfig<C>][0];
type Props = [Omit<ComponentProps, keyof WithSomethingProps>][0];

This causes us not to associate new aliases with the types, and everything reverts back to the 4.1 behavior.

A general fix for this issue unfortunately isn't easy. We only have provisions for associating one type alias with a type so it isn't possible to fall back on an alternative alias. However, one possible solution we could consider is re-aliasing types only when the new alias is at least as reachable as the old alias.