FluidFramework
FluidFramework copied to clipboard
microsoft
Update client to TypeScript 5.3.3
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.
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.
What happens when someone uses TS 5.1? How does it break? How do they know their ts version is the problem?
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)
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.
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.
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.
