Google feedback on TS 4.2-beta

[nreid260]

This GitHub issue contains feedback on the TS 4.2-beta release from the team
that is responsible for keeping Google's internal software working with the
latest version of TypeScript.

Executive summary

• We do not expect to have significant difficulty in upgrading Google to TS
  4.2.
• We found one change to the emitted JS that broke our tsickle and Clutz
  tool's output in some cases. We will fix the tools to match the new
  behavior.
• Some changes to our TypeScript code are required to make it compile with TS
  4.2.
  • About a third of the new errors reported by TS 4.1-beta are clearly
    related to announced changes.
  • The other 60% of new TS error messages to report legitimate source code
    errors that were previously missed by TS 4.1.
  • Detail sections below explain the changes to our code we expect to make
    to unblock the upgrade.

Impact summary

Change description	Announced	Libraries affected
.d.ts Extension Breaks Module Resolution	no	N/A
Changes to lib.d.ts Definitions	no	0.06%
Template Literal Types	yes	0.05%
Function not Called in Condition	no	0.02%
Misc Other Changes	no	0.01%
noImplicitAny in yield	yes	~0
Type Arguments in JavaScript	yes	~0
Primitives as in RHS	yes	~0
abstract Construct Signatures	yes	~0
Typechecking Call Stack Depth Increased	no	~0

The Announced column indicates whether we were able to connect the observed
change with a section in the
TS4.2-beta announcement.

The following sections give more detailed explanations of the changes listed
above.

Template Literal Types

This change was announced.

We find the typing of template literals questionable, since it leads to
unexpected inference and spammy error messages. It's also unclear what the
application of these types is.

Example:

TS2322: Type '`${string}_null.csv` | `${string}_${string}.csv`' is not assignable to type '`${string}_null` | `${string}_${string}`'.
  Type '`${string}_null.csv`' is not assignable to type '`${string}_null` | `${string}_${string}`'.
    Type 'string' is not assignable to type '`${string}_null` | `${string}_${string}`'.
      Type '`${string}_null.csv`' is not assignable to type '`${string}_${string}`'.

458     filename = `${filename}.csv`;

In this case, a template literal type was inferred for a local variable. The
diagnostic is emitted at a reassignment to that variable. The user clearly
intended type string. Several more verbose examples were found.

Our fix for the new type errors will likely be casting to string.

.d.ts Extension Breaks Module Resolution

This change was not announced.

Starting with TS 4.2 the filename extension must not be included in the import
statement. The module resolution algorithm expects to add '.d.ts' itself.

import {Foo} from './foo.d.ts'; // must be changed to './foo'

The issue on our side was that other tools in our toolchain (tsickle and Clutz)
were generating import statements that included .d.ts extensions. These imports
began failing across our codebase such that many types were unavailable and
those libraries had type errors. There was no obvious connection of module
resolution issues.

We have since updated our tools to omit file extensions. These imports also work
against 4.1, so we don't anticipate further issues, regardless of whether this
makes it into 4.2.

Changes to lib.d.ts Definitions

This change was not announced but also expected.

We support the typing improvements. Generally it's clear what's changing.

Our fix will likely be casting away the errors in existing code.

We observed the following breakdown within this category:

• Intl related: 50%
• ResizeObserver: 33%
• Other: 17%

Typechecking Stack Depth Increased

This change was not announced but isn't an API change.

Our fix will likely be to replace some complex types with simpler ones and
accept some losses in type accuracy.

A handful of libraries in our codebase use types that are arguably too complex.
The power of the TS typesystem allows this, but can lead to runaway typing.

A sample stack trace is below:

RangeError: Maximum call stack size exceeded
at createInferenceContext (.../typescript.js:62045:40)
at instantiateSignatureInContextOf (.../typescript.js:68496:27)
at compareSignaturesRelated (.../typescript.js:58654:26)
at signatureRelatedTo (.../typescript.js:60861:24)
at signaturesRelatedTo (.../typescript.js:60811:30)
at structuredTypeRelatedToWorker (.../typescript.js:60214:39)
at structuredTypeRelatedTo (.../typescript.js:59829:30)
at recursiveTypeRelatedTo (.../typescript.js:59801:64)
at isRelatedTo (.../typescript.js:59341:34)
at isPropertySymbolTypeRelated (.../typescript.js:60485:28)

[DanielRosenwasser]

I have not yet read this in full, but thank you all tremendously for the summary on 4.2 which we find invaluable.

We have second guessed the template literal inference changes in 4.2 quite a bit for the last few weeks, and we have a revert out for review here: #42588

[MartinJohns]

Just a small side-note: Your seemingly arbitrary use of linebreaks in the middle of sentences makes reading this issue very difficult.

[DanielRosenwasser]

Your seemingly arbitrary use of linebreaks

I think this is an unfortunate part of GitHub's commenting system, which uses a different markdown parsing strategy from its doc system. When writing markdown, I use one line per sentence. In the write-up @nreid260 had, there seems to be an auto-wrapping strategy. So where most markdown document parsers interpret these as part of the same paragraph, GitHub's comment parser kicks separate lines onto...separate lines, and makes our text look sub-optimal.

[nreid260]

RE: linebreaks, this summary was written first as an internal PR and so got formatted using our Markdown formatter

[DanielRosenwasser]

For what it's worth, I also really enjoy these because it shows me holes in the breaking change section of our release notes. For example, I will be adding in the .d.ts resolution break in the blog post. Thank you for this again!

[nreid260]

For 4.2-rc we observed the following:

Change description	Announced	Libraries affected
Changes to lib.d.ts Definitions	no	0.06%
Template Literal Types	yes	~0
Function not Called in Condition	no	0.03%
Misc Other Changes	no	0.02%
noImplicitAny in yield	yes	~0
Type Arguments in JavaScript	yes	~0
Primitives as in RHS	yes	~0
abstract Construct Signatures	yes	~0
Typechecking Call Stack Depth Increased	no	~0

[DanielRosenwasser]

Since you'd mentioned 50% of breaks being related to Intl - I should mention some of those have been reverted as well. You may need to deal with the removal of certain type aliases that'd been introduced for 4.2, but backed out post-RC.

[evmar]

Thanks for the warning. I think we won't adapt our code to the TS releases until they are final (and maybe sometimes even after a few point releases in some cases), in part because it takes us a while to catch up, but also so we can avoid some of the churn that inevitably happens.

[DanielRosenwasser]

Are you saying that this time it was so bad that you are stopping, or that you only determined the root cause of the breaks for Beta and RC and didn't try fixing them until stable? We really appreciate you all trying these early!

[evmar]

The latter! It takes us a while to adopt a version due to the work involved, so we are still slow. For example, we just rolled out 4.1 recently.

In other words, we are trying to provide feedback on the betas so the feedback comes in early enough to be able to fix any big problems before they're released, but in terms of actually deploying those eventual releases we are likely ~months behind.