Google feedback on TS 5.6-beta

trevorade commented 3 weeks ago

Acknowledgement

[x] I acknowledge that issues using this template may be closed without further explanation at the maintainer's discretion.

Comment

This GitHub issue contains feedback on the TS 5.6-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 5.6.
Some changes to our TypeScript code are required to make it compile with TS 5.6.
We observed regressions in relation to generic type inference (https://github.com/microsoft/TypeScript/issues/59656)
Detail sections below explain the changes to our code we expect to make to unblock the upgrade.

Impact summary

Change description	Announced	Libraries affected
Disallowed Nullish and Truthy Checks	Yes	0.070%
lib.d.ts Changes	Yes	0.004%
Correct override Checks on Computed Properties	Yes	0.004%
Co- vs. contra-variant inference improvements	No	0.004%

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

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

Announced Changes

This section reviews all announced changes from the TS5.6-beta announcement, whether we support these changes at Google, how we will resolve pre-existing issues for these changes (if applicable), and other thoughts.

Disallowed Nullish and Truthy Checks

We support this change. The check uncovers real unintentional runtime behavior.

As part of this migration, we will add // @ts-ignore suppressions to silence pre-existing errors with a note advising code authors to revisit the code and fix these genuine problems.

Includes the following errors: TS2869, TS2870, TS2871, TS2872, TS2873

Iterator Helper Methods

We support this change.

As these are new, none of our codebase in TS utilizes these so nothing was impacted.

However, the wide number of new unique iterators added is somewhat disruptive for our TS to JS interoperability infrastructure but we have a fairly simple workaround for now (e.g. ArrayIterator, MapIterator, SetIterator, StringIterator etc).

Strict Builtin Iterator Checks (and --strictBuiltinIteratorReturn)

We support this change. We typically interact with Iterator types via methods like [...it], Array.from(it), and for (const val of it) {} so this change is less interesting to us.

We did notice some new compiler errors in our copy of the vscode code base but I can see they have been resolved already so we'll update: https://github.com/microsoft/vscode/pull/222009

Support for Arbitrary Module Identifiers

Presently, we do not plan on utilizing this feature in Google. We support TypeScript adding support for this though.

The changes to the TSC API broke a handful of code locations utilizing the TypeScript API that expected an identifier but now have to support string literals. That is, for the code that assumes propertyName in ImportSpecifier and ExportSpecifier is an Identifier will need to be updated to account for ModuleExportName which also accepts a StringLiteral. For now, we will resolve these instances by either throwing an exception for string literal values or emitting a diagnostic (where possible).

The `--noUncheckedSideEffectImports` Option

We support this change.

Note: We already enforced this in our build infrastructure. The error simply shows up earlier now in the stack.

The `--noCheck` Option

We support this change.

Note: This seems useful for incremental development scenarios where we want our edit-refresh cycle to be as short as possible.

Allow `--build` with Intermediate Errors

We support this change.

Note: We do not utilize the --build flag.

Region-Prioritized Diagnostics in Editors

We support this change.

Note: Editor-specific changes don't impact us.

Search Ancestor Configuration Files for Project Ownership

We support this change.

Note: Editor-specific changes don't impact us though.

lib.d.ts Changes

We support this change.

The changes here were pretty sweeping and sometimes at odds with other typings we already had in the codebase.

As part of this migration, we will add // @ts-ignore suppressions to silence pre-existing errors with a note advising code authors to update the code.

It should be noted that there is a lot of noise in the lib.dom.d.ts updates including many dropped MDN comments as well as new ones added. The general order and structure of some types changed as well. This makes is somewhat difficult to identify the meaningful changes.

In addition, it would be helpful for the lib.dom.d.ts file to be directly checked into github so that updates are viewable in the repo itself to detect unwanted diffs. At the moment, this file appears to be generated as part of the TSC release process and so its changes are harder to detect.

.tsbuildinfo is Always Written

We support this change.

Note: We do not utilize the --build flag.

Respecting File Extensions and package.json from within node_modules

We support this change.

Note: We presently don't use .mts nor .cts file extensions though it's probable that we will support these as we start relying on Open Source projects that use them.

Correct override Checks on Computed Properties

We support this change.

This change identified a number of code patterns in our codebase that needed override.

We will resolve these by adding the missing overrides.

Unannounced Changes

Improved logic to chooses co- vs. contra-variant inference

We support this change (provided that observed regressions are resolved).

https://github.com/microsoft/TypeScript/pull/57909 introduced improvements to the compiler that fixes two issues.

We observed several new types of inference issues in our codebase that appear to have stemmed from this.

https://github.com/microsoft/TypeScript/issues/59656 captures one issue where I was able to make a reproduction.

Another class of issues surrounds some of our test infrastructure that uses generics to render templates. We've observed typing errors when passing in object literals containing parameters for the templates. These include optional parameters disappearing or becoming required in the parameter object bag type.

We're hoping that https://github.com/microsoft/TypeScript/pull/59709 resolves both of these classes of issues.

Once that is submitted, we will rebuild the failing targets and report back if the issue persists (hopefully with a simplified reproduction for the second issue).

Andarist commented 3 weeks ago

We're hoping that https://github.com/microsoft/TypeScript/pull/59709 resolves both of these classes of issues.

Would you be able to test this patch using npm:@typescript-deploys/pr-build@5.7.0-pr-59709-8? It would be great to confirm that it resolves all of your issues as early as possible.

trevorade commented 3 weeks ago

Would you be able to test this patch using npm:@typescript-deploys/pr-build@5.7.0-pr-59709-8? It would be great to confirm that it resolves all of your issues as early as possible.

I don't think I can trivially apply that given our toolchain in Google. Is it sufficient if I wait for RC to verify?

Edit: I'm asking the team though for advice on trying to apply this though.

Edit2: Looks like I can locally patch your changes and re-build on our side. I'll come back to this on Monday and report back.

Andarist commented 3 weeks ago

Thanks, I appreciate it! This would just speed up the process and increase the chances that your issues get fixed before the stable release

trevorade commented 2 weeks ago

Hey there. Confirmed that this fix does fix the specific issue noted in https://github.com/microsoft/TypeScript/issues/59656.

That said, I am still seeing other new inference-related bugs that I was hoping this fix would resolve. I'll try to construct repros in TS Playground for those ones and file new issues.

RyanCavanaugh commented 2 weeks ago

Thanks as always!

microsoft / TypeScript