phetsims / chipper

Tools for developing and building PhET interactive simulations.
MIT License
11 stars 12 forks source link

Update to TypeScript 5.4 #1432

Closed samreid closed 2 months ago

samreid commented 2 months ago

Typescript 5.4 was released about a month ago: https://devblogs.microsoft.com/typescript/announcing-typescript-5-4/

It has some improved narrowing, and updated types that will be helpful for our project. To update TypeScript, we would also need to update ESLint + typescript-eslint.

Here is a patch with current versions for these releases:

```diff Subject: [PATCH] Round to nearest thousandth, see https://github.com/phetsims/projectile-data-lab/issues/265 --- Index: package.json IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/package.json b/package.json --- a/package.json (revision aba682fffe1f05b2f2df65781e23123047653db4) +++ b/package.json (date 1712240496564) @@ -27,17 +27,17 @@ "@types/three": "~0.137.0", "@types/react": "~18.0.12", "@types/react-dom": "~18.0.5", - "@typescript-eslint/parser": "~6.18.1", - "@typescript-eslint/eslint-plugin": "~6.18.1", - "@typescript-eslint/utils": "~6.18.1", - "eslint-plugin-html": "~7.1.0", + "@typescript-eslint/parser": "~7.5.0", + "@typescript-eslint/eslint-plugin": "~7.5.0", + "@typescript-eslint/utils": "~7.5.0", + "eslint-plugin-html": "~8.0.0", "@webgpu/types": "~0.1.34", "archiver": "~5.3.0", "axios": "~0.21.4", - "@babel/eslint-parser": "~7.19.1", + "@babel/eslint-parser": "~7.24.1", "docdash": "~1.2.0", - "eslint": "~8.28.0", - "eslint-plugin-react": "~7.31.11", + "eslint": "~8.57.0", + "eslint-plugin-react": "~7.34.1", "expose-loader": "~4.1.0", "grunt": "~1.5.3", "html-webpack-plugin": "~5.3.2", @@ -56,7 +56,7 @@ "taffydb": "~2.7.3", "terser": "~4.6.4", "ts-morph": "^21.0.1", - "typescript": "~5.3.3", + "typescript": "~5.4.3", "webpack": "~5.76.3", "webpack-cli": "~5.0.0", "webpack-dev-server": "~4.11.1" ```

With these new versions, tsc/all has one new type error:

../../../phet-core/js/types/Concat.ts:14:94 - error TS2574: A rest element type must be an array type.

14 type Concat<T> = T extends [ infer A, ...infer Rest ] ? A extends IntentionalAny[] ? [ ...A, ...Concat<Rest> ] : A : T;
                                                                                                ~~~~~~~~~~~~~~~

Found 1 error in ../../../phet-core/js/types/Concat.ts:14

And lint-everything has ✖ 52 problems (51 errors, 1 warning):

``` All results (repeated from above) /Users/samreid/phet/root/alpenglow/js/webgpu/WebGPURecorder.ts 375:24 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion /Users/samreid/phet/root/alpenglow/js/webgpu/wgsl/gpu/loadMultipleWGSL.ts 166:83 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion 171:33 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion 192:39 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion 198:41 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion /Users/samreid/phet/root/alpenglow/js/webgpu/wgsl/gpu/loadReducedWGSL.ts 173:65 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion 174:24 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion 181:83 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion 186:33 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion /Users/samreid/phet/root/bending-light/js/intro/model/IntroModel.ts 321:9 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion /Users/samreid/phet/root/circuit-construction-kit-common/js/model/analysis/LTACircuitTests.ts 133:27 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion /Users/samreid/phet/root/circuit-construction-kit-common/js/view/ValueNode.ts 246:7 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion 277:57 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion 278:34 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion 279:32 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion 280:50 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion /Users/samreid/phet/root/joist/js/Helper.ts 2035:52 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion /Users/samreid/phet/root/joist/js/KeyboardHelpDialog.ts 102:34 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion /Users/samreid/phet/root/kite/js/ops/SegmentTree.ts 445:25 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion /Users/samreid/phet/root/molecule-polarity/js/common/view/PointySliderThumb.ts 68:26 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion /Users/samreid/phet/root/natural-selection/js/common/model/parseInitialPopulation.ts 53:27 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion /Users/samreid/phet/root/number-suite-common/js/common/view/CountingAccordionBox.ts 87:29 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion /Users/samreid/phet/root/phet-io-wrappers/common/js/migration/MigrationPhetioAPI.ts 81:12 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion 86:12 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion 93:12 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion 99:12 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion 104:12 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion /Users/samreid/phet/root/phet-io-wrappers/common/js/migration/processors/report-processors/RemoveExpectedReportErrors.ts 78:32 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion /Users/samreid/phet/root/phetmarks/js/phetmarks.ts 1069:30 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion /Users/samreid/phet/root/quadrilateral/js/quadrilateral/model/QuadrilateralSoundOptionsModel.ts 31:28 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion /Users/samreid/phet/root/quadrilateral/js/quadrilateral/model/QuadrilateralUtils.ts 176:84 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion 179:85 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion /Users/samreid/phet/root/quadrilateral/js/quadrilateral/view/QuadrilateralAlerter.ts 885:25 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion /Users/samreid/phet/root/quadrilateral/js/quadrilateral/view/prototype/QuadrilateralTangibleController.ts 133:42 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion 134:42 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion 135:42 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion 136:42 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion 143:41 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion 144:41 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion 145:41 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion 146:41 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion /Users/samreid/phet/root/scenery-phet/js/SpectrumSlider.ts 410:26 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion /Users/samreid/phet/root/scenery-phet/js/SpectrumSliderThumb.ts 83:26 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion /Users/samreid/phet/root/scenery/js/accessibility/pdom/ParallelDOMTests.ts 1795:25 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion /Users/samreid/phet/root/scenery/js/input/Input.ts 253:31 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion 1189:28 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion /Users/samreid/phet/root/scenery/js/listeners/KeyboardListener.ts 524:20 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion 531:32 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion /Users/samreid/phet/root/scenery/js/listeners/PressListener.ts 658:14 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion /Users/samreid/phet/root/scenery/js/tests/PixelComparisonTests.js 250:3 warning Unused eslint-enable directive (no matching eslint-disable directives were found) /Users/samreid/phet/root/tandem/js/types/EnumerationIO.ts 46:16 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion /Users/samreid/phet/root/tandem/js/types/StateSchema.ts 282:14 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion ✖ 52 problems (51 errors, 1 warning) 51 errors and 1 warning potentially fixable with the `--fix` option. Fatal error: Lint failed Fatal error: perennial grunt "--repo=projectile-data-lab" "lint-everything" "--disable-eslint-cache" failed with code 1 ``` I think we should take a closer look.
samreid commented 2 months ago

I believe we can fix the type error in Concat with a conditional type check, like so:

Subject: [PATCH] Round to nearest thousandth, see https://github.com/phetsims/projectile-data-lab/issues/265
---
Index: js/types/Concat.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/types/Concat.ts b/js/types/Concat.ts
--- a/js/types/Concat.ts    (revision dcab3dec9107be23c3599468a095d34680c37640)
+++ b/js/types/Concat.ts    (date 1712241704251)
@@ -11,5 +11,5 @@
  * @author Jonathan Olson <jonathan.olson@colorado.edu>
  */

-type Concat<T> = T extends [ infer A, ...infer Rest ] ? A extends IntentionalAny[] ? [ ...A, ...Concat<Rest> ] : A : T;
+type Concat<T> = T extends [ infer A, ...infer Rest ] ? A extends IntentionalAny[] ? [ ...A, ...( Concat<Rest> extends IntentionalAny[] ? Concat<Rest> : [] ) ] : A : T;
 export default Concat;
\ No newline at end of file

Please note that Concat was added in Feb 2022 and has 0 usages across our project at the moment.

samreid commented 2 months ago

The lint changes are all from typescript's improved type inference ability. For example, the first case is WebGPURecorder.ts:

      Object.keys( value ).forEach( key => {
        if ( objectOverrides && objectOverrides[ key ] ) {
          map[ key ] = objectOverrides[ key ]!( value[ key ] );
        }

The type assertion is no longer necessary since typescript knows objectOverrides[key] exists from the previous line.

Likewise, the first error in Circuit Construction Kit:

    update = () => {

      readoutValueNode!.visible = showValuesProperty.value && circuitElement.isValueDisplayableProperty.value;

It now knows that readoutValueNode is narrowed to Node so the type assertion is no longer needed there.

There are all good reasons to move to TypeScript 5.4.

samreid commented 2 months ago

@zepumph and I committed this, and it seems good. Closing.

zepumph commented 2 months ago

I was very happy to see the changes that this new version brought. Type checking definitely got smarting. Thanks TypeScript!