Google feedback on TS 4.2-beta #42593
Description
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.
- About a third of the new errors reported by TS 4.1-beta are clearly
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)
Activity
DanielRosenwasser commentedon Feb 1, 2021
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 commentedon Feb 2, 2021
Just a small side-note: Your seemingly arbitrary use of linebreaks in the middle of sentences makes reading this issue very difficult.
DanielRosenwasser commentedon Feb 2, 2021
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 Nick Reid (@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 commentedon Feb 2, 2021
RE: linebreaks, this summary was written first as an internal PR and so got formatted using our Markdown formatter
DanielRosenwasser commentedon Feb 10, 2021
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.tsresolution break in the blog post. Thank you for this again!nreid260 commentedon Feb 16, 2021
For 4.2-rc we observed the following:
DanielRosenwasser commentedon Feb 23, 2021
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 commentedon Feb 24, 2021
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 commentedon Feb 24, 2021
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 commentedon Feb 24, 2021
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.