chore: upgrade typescript to 5.5.2 #33498
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fabricteam
reviewed
Dec 20, 2024
| @@ -69,8 +69,8 @@ | |||
| "@griffel/webpack-loader": "2.2.10", | |||
There was a problem hiding this comment.
🕵🏾♀️ visual regressions to review in the fluentuiv9 Visual Regression Report
Avatar Converged 1 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| Avatar Converged.Badge Mask RTL.chromium.png | 42 | Changed |
🕵 FluentUIV0 No visual regressions between this PR and main |
mainframev
commented
Dec 20, 2024
| @@ -85,7 +91,9 @@ export function classNamesFunction<TStyleProps extends {}, TStyleSet extends ISt | |||
| typeof styleFunctionOrObject === 'function' && | |||
| (styleFunctionOrObject as StyleFunction<TStyleProps, TStyleSet>).__noStyleOverride__ | |||
| ) { | |||
| return styleFunctionOrObject(styleProps) as IProcessedStyleSet<TStyleSet>; | |||
| return (styleFunctionOrObject as IStyleFunction<TStyleProps, TStyleSet>)( | |||
There was a problem hiding this comment.
had to add additional casting, throwing after upgrade with this TS2349:
| state.components?.[slotName] === undefined || typeof state.components[slotName] === 'string' | ||
| ? asProp || state.components?.[slotName] || 'div' | ||
| : state.components[slotName] | ||
| ) as React.ElementType<R[K]>; | ||
| : state.components[slotName]; |
There was a problem hiding this comment.
after upgrade throws TS2590: Expression produces a union type that is too complex to represent
There was a problem hiding this comment.
that's strange, there shouldn't be difference between asserting on definition vs this artificial alias you created. can you double check please ?
mainframev
force-pushed
the
chore/ts-update-5-5
branch
2 times, most recently
from
29cadea to
ea576b0
Compare
mainframev
commented
Dec 20, 2024
packages/codemods/package.json
Outdated
| @@ -31,6 +31,7 @@ | |||
| "tslib": "^2.1.0", | |||
| "react": "17.0.2", | |||
| "ts-morph": "^10.0.1", | |||
| "typescript": "5.3.3", | |||
There was a problem hiding this comment.
had to lock ts to 5.3.3 for this package, as ts-morph has issue with 5.4: dsherret/ts-morph#1513. Fixed in v22, current is 10 😢
mainframev
force-pushed
the
chore/ts-update-5-5
branch
from
ea576b0 to
3ee9cbd
Compare
fabricteam
reviewed
Dec 20, 2024
| @@ -0,0 +1,7 @@ | |||
| { | |||
There was a problem hiding this comment.
🕵🏾♀️ visual regressions to review in the fluentuiv8 Visual Regression Report
Callout 5 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| Callout.Bottom Right Edge RTL.chromium.png | 1593 | Changed |
| Callout.No callout width specified.chromium.png | 2319 | Changed |
| Callout.Top auto edge.chromium.png | 2307 | Changed |
| Callout.Right center.chromium.png | 2396 | Changed |
| Callout.Gap space 25.chromium.png | 2309 | Changed |
|
Pull request demo site: URL |
mainframev
force-pushed
the
chore/ts-update-5-5
branch
from
3ee9cbd to
83e4110
Compare
📊 Bundle size report
Unchanged fixtures
|
mainframev
force-pushed
the
chore/ts-update-5-5
branch
2 times, most recently
from
0e865c5 to
0a3b8eb
Compare
mainframev
force-pushed
the
chore/ts-update-5-5
branch
3 times, most recently
from
9f52366 to
415da9a
Compare
chore(utilities): update api.md chore(react-utilities): update api.md chore(style-utilities): update api.md
mainframev
force-pushed
the
chore/ts-update-5-5
branch
from
415da9a to
eccd0d4
Compare
🕵 fluentui-web-components-v3 No visual regressions between this PR and main |
mainframev
commented
Dec 27, 2024
| @@ -3,6 +3,9 @@ const { createV8Config: createConfig } = require('@fluentui/scripts-jest'); | |||
| const config = createConfig({ | |||
| setupFiles: ['./config/tests.js'], | |||
| snapshotSerializers: ['@fluentui/jest-serializer-merge-styles'], | |||
| // Keeps Jest from using too much memory as GC gets invokes more often, makes tests slower | |||
| // https://stackoverflow.com/a/75857711 | |||
| workerIdleMemoryLimit: '1024MB', | |||
There was a problem hiding this comment.
unblocking pipeline, fails on OOM same as here: #33480
There was a problem hiding this comment.
how slow are we talking ? is this caused by the perf downgrade of TS ? (v8 is using ts-jest => typescript)
can you turn off type-checking first to trouble-shoot further for v8 ?
mainframev
requested review from
a team,
Mitch-At-Work,
mltejera,
marcosmoura,
GeoffCoxMSFT,
chpalac,
ling1726,
dmytrokirpa,
behowell,
spmonahan,
khmakoto and
dzearing
as code owners
Hotell
reviewed
Jan 2, 2025
| @@ -25,7 +25,8 @@ | |||
| "devDependencies": { | |||
| "@fluentui/eslint-plugin": "*", | |||
| "@fluentui/scripts-jest": "*", | |||
| "@fluentui/scripts-tasks": "*" | |||
| "@fluentui/scripts-tasks": "*", | |||
| "typescript": "5.3.3" | |||
There was a problem hiding this comment.
we don't wanna maintain various TS versions in monorepo. for short term it's ok
-> please create issue to bump ts-morph in this package to get rid of this TS dep
-> if the upgrade of ts-morph is easy enough we can do it as part of this PR
sidenote: this package should be probably removed completely as it only use was for v8 AFAIK and has not been updated for quite some time , so if the efforts of bumping ts-morph are huge better investment is to remove this package completely
| /** @typedef { import('@typescript-eslint/utils').TSESTree.VariableDeclarator } VariableDeclarator*/ | ||
| /** @typedef { import('@typescript-eslint/utils').TSESTree.ExportSpecifier} ExportSpecifier */ | ||
| /** | ||
| * @import { TSESTree } from '@typescript-eslint/utils' |
There was a problem hiding this comment.
this was not necessary for the actual TS migration right ? it's a very welcomed addition indeed but next time we should do this in a separate PR
| export const ScaleSnappy: PresenceComponent< { | ||
| animateOpacity?: boolean | undefined; | ||
| }>; | ||
| export const ScaleSnappy: PresenceComponent<ScaleRuntimeParams_unstable>; |
There was a problem hiding this comment.
hmm, interesting behaviour. is this documented ? it seems like breaking change within API-extractor.
also if this .md relfects the actual rolluped index.d.ts than this is invalid type definition - there is no ScaleRuntimeParams_unstable import 🚨 ( previously it unwrapped the alias )
| @@ -31,7 +31,7 @@ export interface OnOverflowChangeData extends OverflowState { | |||
| // @public | |||
| export const Overflow: React_2.ForwardRefExoticComponent<Partial<Pick<ObserveOptions, "padding" | "overflowDirection" | "overflowAxis" | "minimumVisible">> & { | |||
| children: React_2.ReactElement; | |||
| onOverflowChange?: ((ev: null, data: OverflowState) => void) | undefined; | |||
| onOverflowChange?: (ev: null, data: OverflowState) => void; | |||
There was a problem hiding this comment.
this might be a breaking change as the previous output supported passing
onOverflowChange: undefined , with this it will throw Type error if explicit undefined is present. can you verify this please ?
| state.components?.[slotName] === undefined || typeof state.components[slotName] === 'string' | ||
| ? asProp || state.components?.[slotName] || 'div' | ||
| : state.components[slotName] | ||
| ) as React.ElementType<R[K]>; | ||
| : state.components[slotName]; |
There was a problem hiding this comment.
that's strange, there shouldn't be difference between asserting on definition vs this artificial alias you created. can you double check please ?
| @@ -3,6 +3,9 @@ const { createV8Config: createConfig } = require('@fluentui/scripts-jest'); | |||
| const config = createConfig({ | |||
| setupFiles: ['./config/tests.js'], | |||
| snapshotSerializers: ['@fluentui/jest-serializer-merge-styles'], | |||
| // Keeps Jest from using too much memory as GC gets invokes more often, makes tests slower | |||
| // https://stackoverflow.com/a/75857711 | |||
| workerIdleMemoryLimit: '1024MB', | |||
There was a problem hiding this comment.
how slow are we talking ? is this caused by the perf downgrade of TS ? (v8 is using ts-jest => typescript)
can you turn off type-checking first to trouble-shoot further for v8 ?
| @@ -5,7 +5,6 @@ exports.register = register; | |||
| */ | |||
| function register() { | |||
| if (process.env.NODE_ENV !== 'test') { | |||
| // @ts-expect-error - ts-node/register doesn't provide types so this would error | |||
There was a problem hiding this comment.
how come this is not needed anymore ?
Comment on lines
+5
to
+8
| /** @import { PackageJson } from './types'; | ||
| * @import { ProjectGraph } from '@nx/devkit'; | ||
| */ | ||
|
|
There was a problem hiding this comment.
this might be invalid JSDoc
Suggested change
| /** @import { PackageJson } from './types'; | |
| * @import { ProjectGraph } from '@nx/devkit'; | |
| */ | |
| /** | |
| * @import { PackageJson } from './types'; | |
| * @import { ProjectGraph } from '@nx/devkit'; | |
| */ | |
| @@ -15,7 +15,11 @@ export function concatStyleSetsWithProps<TStyleProps, TStyleSet extends IStyleSe | |||
| const result: Array<DeepPartial<TStyleSet>> = []; | |||
| for (const styles of allStyles) { | |||
| if (styles) { | |||
| result.push(typeof styles === 'function' ? styles(styleProps) : styles); | |||
| if (typeof styles === 'function') { | |||
| result.push((styles as IStyleFunction<TStyleProps, TStyleSet>)(styleProps)); | |||
There was a problem hiding this comment.
we can add these assertions within original ternary = keeping the bundle size on same level
| resolve "~1.22.1" | ||
| semver "~7.5.4" | ||
| source-map "~0.6.1" | ||
| typescript "5.3.3" | ||
| typescript "5.4.2" |
There was a problem hiding this comment.
this might be crucial for us - if they don't support 5.5 it might affect our public APIs that we ship (see my comments in this PR). if that's the case we are forced to bump only to ts 5.4
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@importJSDoc tags.Release notes
https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-4.html
https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-5.html
Related to configDir template var: the initial assumption, that it will help to get rid of explicit typeRoots for
tsconfig.cy.jsonfiles appeared to be wrong. It works for scenarios, where derived config (packages//tsconfig.json) has to have paths related to final directory, before it was needed to re-specify the config property for derived config.Previous Behavior
5.3.3
New Behavior
5.5.2
Related Issue(s)
(nx run-many -t build type-check --skip-nx-cache)