Update client to TypeScript 5.3.3 #20994

CraigMacomber · 2024-05-06T18:27:19Z

Description

Typescript 5.3 brings useful features like support for Symbol.hasInstance narrowing, type checking optimizations and much more. Since ClientRequirements.md lists our requirement at TypeScript 5.4, this should be ok (from the perspective of aligning with that requirement)

See https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-2.html and https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-3.html for more details.

Based on a two test runs, this change makes pnpm run build:compile about 7% faster. (105.438 and 107.011 seconds instead of 113.816 and 114.096 without the change)

Breaking Changes

This is not expected to break anything, however its possible that the compile TypeScript is slightly different, which could cause issues in some edge cases.

This will enable code to start relying on newer features, which when used in public APIs could in turn cause breakages for users of older versions of TypeScript.

There are a couple cases where type-tests were impacted. These seemed to be a fix of typescript capturing more accurate types, especially around index signatures, and cases where the TypeScript compiler wouldn't provide any useful information about what changed with the only difference being inside the ... non-printed parts of the types. As the changes seem relatively minor they seem unlikely to cause any real issues (didn't break lots of examples/tests) and since we only support customers using TypeScript 5.4 and plan to move to 5.4 ourselves at some point, if this causes a breaking change in some edge case, that probably a good thing to get out of the way now while we are still in the RC phase.

Reviewer Guidance

The review process is outlined on this wiki page.

Does this cause issues with Fluid-Build's incremental build logic that needs fixing?
Is there anything else that should be validated?

When running build:compile with no changes needing building, it used to run 417 tasks, and now it runs 425. The reason for the difference is unclear, but it still takes 16 seconds (Updated version was 15.5 seconds, old one 16.2). The extra tasks are

[ 1/425] ✓ @fluid-experimental/property-properties: tsc --project ./tsconfig.esnext.json - 0.449s
[ 2/425] ✓ @fluid-experimental/property-properties: tsc - 0.490s
[ 3/425] - @fluid-experimental/property-proxy: fluid-tsc commonjs --project ./tsconfig.cjs.json - 0.007s
[ 4/425] - @fluid-experimental/property-proxy: tsc --project ./tsconfig.json - 0.009s
[ 5/425] - @fluid-experimental/property-proxy: copyfiles -f ../../../../common/build/build-common/src/cjs/package.json ./dist - 0.001s
[ 21/425] ✓ @fluid-experimental/property-proxy: tsc --project ./src/test/tsconfig.json - 0.537s
[ 22/425] ✓ @fluid-experimental/property-properties: tsc --project ./src/test/tsconfig.json - 0.718s
[ 23/425] ✓ @fluid-experimental/property-proxy: fluid-tsc commonjs --project ./src/test/tsconfig.cjs.json - 0.695s

An explanation of why this change occurred might be worth understanding.

CraigMacomber · 2024-05-06T19:49:45Z

examples/data-objects/monaco/webpack.config.cjs

-								loader: "ts-loader",
-								options: {
-									compilerOptions: {
-										module: "esnext",


This config caused TS5110: Option 'module' must be set to 'Node16' when option 'moduleResolution' is set to 'Node16'. So I made it based on our default config. The dependencies still cause source maps issues which make the app not work when manually testing it), so I had to keep source map suppression.

CraigMacomber · 2024-05-06T19:50:25Z

packages/dds/sequence/api-report/sequence.api.md

@@ -317,7 +317,7 @@ export type IntervalStickiness = (typeof IntervalStickiness)[keyof typeof Interv
 export enum IntervalType {
    // (undocumented)
    Simple = 0,
-    SlideOnRemove = 2,
+    SlideOnRemove = 2,// SlideOnRemove is default behavior - all intervals are SlideOnRemove


New typescript seems to result in preserved trailing comments in some cases like this. Interesting, but I think a non-issue.

I seem to remember @DLehenbauer mentioning that we should try to upgrade api-extractor and typescript at the same time and I think these sorts of things was one reason. Though @Josmithr has also updated api-extractor independently so I could be way off here.

We are already using a version of API extractor which uses newer typescript than we build with (and thats normal/fine as far as I understand). This change brings them in alignment again (if our typescript is newer than theirs, we get warnings).

There also is a desire to update API extractor, which would use typescript 5.4: we can't do that as some of our internal APIs fail to compiler with typescript 4.5 due to issues with typeBox (thats one of the reasons I picked 5.3 as a target)

CraigMacomber · 2024-05-06T19:51:43Z

packages/framework/fluid-static/package.json

@@ -140,6 +140,34 @@
 			"TypeAliasDeclaration_Myself": {
 				"forwardCompat": false,
 				"backCompat": false
+			},
+			"InterfaceDeclaration_IFluidContainer": {


The API breakages seem to be (for the ones I looked at in detail anyway) improved type safety catching more incompatibilities from the IFLuidHandle refactor which were previously missed.

CraigMacomber · 2024-05-06T19:54:59Z

packages/dds/tree/src/test/feature-libraries/optional-field/optionalFieldUtils.ts

@@ -152,7 +152,7 @@ export const Change = {
 		const changeset: Mutable<OptionalChangeset> = { moves, childChanges };
 		for (const changeLike of changes) {
 			if ("type" in changeLike === false) {
-				const change = changeLike as OptionalChangeset;
+				const change = changeLike;


improved narrowing made this as cast a no-op, so it had to be removed as per our compiler settings.

CraigMacomber · 2024-05-06T19:55:47Z

packages/drivers/odsp-driver/src/odspDelayLoadedDeltaStream.ts

@@ -147,7 +147,7 @@ export class OdspDelayLoadedDeltaStream {
 			const websocketTokenPromise = requestWebsocketTokenFromJoinSession
 				? // eslint-disable-next-line unicorn/no-null
 				  Promise.resolve(null)
-				: this.getWebsocketToken!(options);
+				: this.getWebsocketToken(options);


improved narrowing removed the new for this non-null assertion which thus had to be removed due to our compiler settings.

…to ClientTS

tylerbutler · 2024-05-07T13:53:11Z

What happens when someone uses TS 5.1? How does it break? How do they know their ts version is the problem?

CraigMacomber · 2024-05-07T17:14:40Z

What happens when someone uses TS 5.1? How does it break? How do they know their ts version is the problem?

If the only feature we use from 5.3 in public APIs is the improved HasInstance based narrowing (which is the thing I currently want to use) then what would break is adding new usages of instanceof in some specific places would start narrowing correctly once we added the corresponding implementations, but only for TS 5.3+.

Currently I don't expect it this change to break anything but it will be possible for people to start relying on newer TS features, and when they do so in public APIs they will have to be careful to try and make any errors users of older versions will get intelligible errors. In the past we have done similar changes without even documenting the required TS version (at least not anywhere that I found): at least that has been improved (slightly), but more work is needed to make that version more discoverable (it should be referenced from our readme boilerplate somehow, I suspect. I'm not clear on how to link docs from readmes such that the proper branch will be reference so we don't accidently imply incorrect requirements in the future when 3.0 comes out with different requires and 2.0 still links the docs on main)

…to ClientTS

…ework into ClientTS

ChumpChief

⁠We talked about this in API council yesterday - the only concern raised was whether customers on older versions of TS might have trouble consuming .d.ts files we make using newer TS (even if not on day 0, maybe later as we start taking dependencies on new features).

Have you done research on current versions of TS used by our customers and/or do we know what usage distribution looks like for the various TS versions?

I'm in favor of this change on a personal level, but would be good to have confidence that it won't become disruptive to customers.

Josmithr · 2024-05-08T21:40:16Z

⁠We talked about this in API council yesterday - the only concern raised was whether customers on older versions of TS might have trouble consuming .d.ts files we make using newer TS (even if not on day 0, maybe later as we start taking dependencies on new features).

Have you done research on current versions of TS used by our customers and/or do we know what usage distribution looks like for the various TS versions?

I'm in favor of this change on a personal level, but would be good to have confidence that it won't become disruptive to customers.

We do now have documentation noting that TS 5.4 is a minimal requirement: https://github.com/microsoft/FluidFramework/blob/main/ClientRequirements.md. I would argue that the time to do that sort of investigation is when updating our min requirements, rather than when updating dependencies in accordance with those requirements.

If we want to retain compatibility with earlier versions of TS and avoid consumer disruption for users upgrading to FF2.0, we will need to update our documented min. But I also think we need to be somewhat aggressive when increment our major versions, otherwise we will be stuck on very old versions for a long time.

CraigMacomber · 2024-05-08T21:54:26Z

Have you done research on current versions of TS used by our customers and/or do we know what usage distribution looks like for the various TS versions?

I had Nick run our ClientRequirements.md document (which says we require 5.4 from our customers) by various people for review and got no pushback on that. Given that we already require TS > 5, and there aren't and major compat issues with updating TS from 5.1 to 5.3 that I'm aware of I don't expect this to cause significant issues.

I imagine these stats are not super useful, but in the last 7 days npm shows almost 5 million downloads of the 5.3.3 version this targets, so its the most popular version in the 5.0 - 5.3 range by a significant margin: not many people that bothered getting onto 5 stopped at 5.1 or 5.2. 5.4.5 is the most popular version (over 9 million downloads).

api-extractor moved to ts 5.3.3 5 months ago in 7.39, and that was the first version of it that moved past 5.0, so there are no api-extractor releases supporting our current 5.1 that don't support 5.3.

ChumpChief · 2024-05-09T00:41:12Z

I would argue that the time to do that sort of investigation is when updating our min requirements, rather than when updating dependencies in accordance with those requirements.

I agree with this, so if the investigation was done that's great - I just haven't seen it. EDIT: Craig's response popped up after I posted, thank you Craig!

My point isn't to say "don't move forward if it would disrupt" - my point is to say "we should know how disruptive it will be if we move forward". So that whatever decision we make can be informed by customer impact.

nmsimons

We've vetted this requirement with Hexagon and Autodesk as well as internal SharePoint pages. Consensus was that this is a reasonable requirement.

…to ClientTS

Josmithr · 2024-05-20T17:32:12Z

examples/data-objects/monaco/webpack.config.cjs

+		},
+		plugins: [
+			new MonacoWebpackPlugin(),
+			// new BundleAnalyzerPlugin()


Nit: while we're here

Suggested change

// new BundleAnalyzerPlugin()

Package.json does not even depend on the dep needed to provide this, so ya, seems good to remove it. Done.

## Description Typescript 5.3 brings useful features like support for [Symbol.hasInstance narrowing](https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-3.html#instanceof-narrowing-through-symbolhasinstance), type checking optimizations and much more. Since ClientRequirements.md lists our requirement at TypeScript 5.4, this should be ok (from the perspective of aligning with that requirement) See https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-2.html and https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-3.html for more details. Based on a two test runs, this change makes `pnpm run build:compile` about 7% faster. (105.438 and 107.011 seconds instead of 113.816 and 114.096 without the change) ## Breaking Changes This is not expected to break anything, however its possible that the compile TypeScript is slightly different, which could cause issues in some edge cases. This will enable code to start relying on newer features, which when used in public APIs could in turn cause breakages for users of older versions of TypeScript. There are a couple cases where type-tests were impacted. These seemed to be a fix of typescript capturing more accurate types, especially around index signatures, and cases where the TypeScript compiler wouldn't provide any useful information about what changed with the only difference being inside the `...` non-printed parts of the types. As the changes seem relatively minor they seem unlikely to cause any real issues (didn't break lots of examples/tests) and since we only support customers using TypeScript 5.4 and plan to move to 5.4 ourselves at some point, if this causes a breaking change in some edge case, that probably a good thing to get out of the way now while we are still in the RC phase.

Update client to TypeScript 5.3.3

fcc7441

Fix monaco webpack config

4061228

CraigMacomber commented May 6, 2024

View reviewed changes

CraigMacomber marked this pull request as ready for review May 6, 2024 21:01

CraigMacomber requested review from a team as code owners May 6, 2024 21:01

CraigMacomber added 3 commits May 6, 2024 17:56

Merge branch 'main' of https://github.com/microsoft/FluidFramework in…

018122c

…to ClientTS

Merge branch 'main' of https://github.com/microsoft/FluidFramework in…

f99a4b7

…to ClientTS

Merge branch 'main' into ClientTS

ec030c8

CraigMacomber added 2 commits May 8, 2024 10:05

Merge branch 'main' of https://github.com/microsoft/FluidFramework in…

b16484c

…to ClientTS

Merge branch 'ClientTS' of https://github.com/CraigMacomber/FluidFram…

6994afe

…ework into ClientTS

ChumpChief reviewed May 8, 2024

View reviewed changes

nmsimons approved these changes May 17, 2024

View reviewed changes

CraigMacomber added 7 commits May 17, 2024 16:22

Merge branch 'main' of https://github.com/microsoft/FluidFramework in…

391c742

…to ClientTS

update from merge

50f7139

Some type test fixes

9cf9b29

Fix type tests

daed8c9

Merge branch 'main' into ClientTS

c58293a

Add changeset

90490b4

Merge branch 'main' into ClientTS

eeddd88

Josmithr reviewed May 20, 2024

View reviewed changes

Josmithr approved these changes May 20, 2024

View reviewed changes

Remove commented out BundleAnalyzerPlugin

fc62394

CraigMacomber enabled auto-merge (squash) May 20, 2024 18:09

CraigMacomber merged commit 7ac0f4f into microsoft:main May 20, 2024
30 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update client to TypeScript 5.3.3 #20994

Update client to TypeScript 5.3.3 #20994

CraigMacomber commented May 6, 2024 •

edited

CraigMacomber May 6, 2024 •

edited

CraigMacomber May 6, 2024

tylerbutler May 8, 2024

CraigMacomber May 8, 2024 •

edited

CraigMacomber May 6, 2024

CraigMacomber May 6, 2024

CraigMacomber May 6, 2024

tylerbutler commented May 7, 2024

CraigMacomber commented May 7, 2024

ChumpChief left a comment

Josmithr commented May 8, 2024 •

edited

CraigMacomber commented May 8, 2024

ChumpChief commented May 9, 2024 •

edited

nmsimons left a comment

Josmithr May 20, 2024

CraigMacomber May 20, 2024

Update client to TypeScript 5.3.3 #20994

Update client to TypeScript 5.3.3 #20994

Conversation

CraigMacomber commented May 6, 2024 • edited

Description

Breaking Changes

Reviewer Guidance

CraigMacomber May 6, 2024 • edited

Choose a reason for hiding this comment

CraigMacomber May 6, 2024

Choose a reason for hiding this comment

tylerbutler May 8, 2024

Choose a reason for hiding this comment

CraigMacomber May 8, 2024 • edited

Choose a reason for hiding this comment

CraigMacomber May 6, 2024

Choose a reason for hiding this comment

CraigMacomber May 6, 2024

Choose a reason for hiding this comment

CraigMacomber May 6, 2024

Choose a reason for hiding this comment

tylerbutler commented May 7, 2024

CraigMacomber commented May 7, 2024

ChumpChief left a comment

Choose a reason for hiding this comment

Josmithr commented May 8, 2024 • edited

CraigMacomber commented May 8, 2024

ChumpChief commented May 9, 2024 • edited

nmsimons left a comment

Choose a reason for hiding this comment

Josmithr May 20, 2024

Choose a reason for hiding this comment

CraigMacomber May 20, 2024

Choose a reason for hiding this comment

CraigMacomber commented May 6, 2024 •

edited

CraigMacomber May 6, 2024 •

edited

CraigMacomber May 8, 2024 •

edited

Josmithr commented May 8, 2024 •

edited

ChumpChief commented May 9, 2024 •

edited