Migrate keycodes package to TypeScript

[im3dabasia]

What?

Part of: #67691
Migrating the keycodes package to Typescript.

Why?

Type safety.

Testing Instructions

Typecheck and unit tests.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: im3dabasia <[email protected]>
Co-authored-by: manzoorwanijk <[email protected]>
Co-authored-by: sirreal <[email protected]>
Co-authored-by: tyxla <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[manzoorwanijk]

This looks good to me. Thank you for working on this.

I would like @jsnajdr @tyxla @gziolo to have another look.

[tyxla]

Unfortunately, I'm a bit away from the typing work these days. I'd appreciate it if one of the other pinged folks could have a look. Or maybe @sirreal would have some spare cycles?

[sirreal]

Thanks for the work on this! Overall it looks good, I left a few suggestions to improve some things.

[sirreal]

The file rename unfortunately does not pick up the diff.

It appears that most of the changes are adding as Window type assertions to the test values passed in to isAppleOs.

diff at 05d3d3e

diff --git a/packages/keycodes/src/test/platform.js b/packages/keycodes/src/test/platform.ts
index 63cd2628f98..8a91afe824f 100644
--- a/packages/keycodes/src/test/platform.js
+++ b/packages/keycodes/src/test/platform.ts
@@ -5,50 +5,52 @@ import { isAppleOS } from '../platform';
 
 describe( 'isAppleOS helper', () => {
 	it( 'should identify anything with "Mac" in it as Apple OS', () => {
-		expect( isAppleOS( { navigator: { platform: 'Mac' } } ) ).toEqual(
-			true
-		);
-		expect( isAppleOS( { navigator: { platform: 'MacIntel' } } ) ).toEqual(
-			true
-		);
+		expect(
+			isAppleOS( { navigator: { platform: 'Mac' } } as Window )
+		).toEqual( true );
+		expect(
+			isAppleOS( { navigator: { platform: 'MacIntel' } } as Window )
+		).toEqual( true );
 	} );
 
 	it( 'should identify "iPad" as Apple OS', () => {
-		expect( isAppleOS( { navigator: { platform: 'iPad' } } ) ).toEqual(
-			true
-		);
+		expect(
+			isAppleOS( { navigator: { platform: 'iPad' } } as Window )
+		).toEqual( true );
 	} );
 
 	it( 'should identify "iPhone" as Apple OS', () => {
-		expect( isAppleOS( { navigator: { platform: 'iPhone' } } ) ).toEqual(
-			true
-		);
+		expect(
+			isAppleOS( { navigator: { platform: 'iPhone' } } as Window )
+		).toEqual( true );
 	} );
 
 	it( 'should not identify Windows as MacOS', () => {
-		expect( isAppleOS( { navigator: { platform: 'Windows' } } ) ).toEqual(
-			false
-		);
-		expect( isAppleOS( { navigator: { platform: 'Win' } } ) ).toEqual(
-			false
-		);
+		expect(
+			isAppleOS( {
+				navigator: { platform: 'Windows' },
+			} as Window )
+		).toEqual( false );
+		expect(
+			isAppleOS( { navigator: { platform: 'Win' } } as Window )
+		).toEqual( false );
 	} );
 
 	it( 'should not identify *NIX as MacOS', () => {
-		expect( isAppleOS( { navigator: { platform: 'Linux' } } ) ).toEqual(
-			false
-		);
-		expect( isAppleOS( { navigator: { platform: 'Unix' } } ) ).toEqual(
-			false
-		);
+		expect(
+			isAppleOS( { navigator: { platform: 'Linux' } } as Window )
+		).toEqual( false );
+		expect(
+			isAppleOS( { navigator: { platform: 'Unix' } } as Window )
+		).toEqual( false );
 	} );
 
 	it( 'should not identify other cases as MacOS', () => {
-		expect( isAppleOS( { navigator: { platform: 'MAC' } } ) ).toEqual(
-			false
-		);
-		expect( isAppleOS( { navigator: { platform: 'mac' } } ) ).toEqual(
-			false
-		);
+		expect(
+			isAppleOS( { navigator: { platform: 'MAC' } } as Window )
+		).toEqual( false );
+		expect(
+			isAppleOS( { navigator: { platform: 'mac' } } as Window )
+		).toEqual( false );
 	} );
 } );

[im3dabasia]

Would it be wise to leave the tests untouched? I see plenty of packages which are in TS have their tests in JS. WDYT?

[sirreal]

To clarify, I commented and shared the diff to document the changes for posterity since it shows up as a delete/add in GitHub UI.

In this situation where every test requires an assertion and nothing else is changed, I would probably have left the file unchanged. I'll leave this case up to you.

[sirreal]

This looks like it changed in this PR, before T extended { [s: string]: any; } | ArrayLike<any> in the Template, here it's Record< string, any >.

This function is internal and only appears to be passed the modifiers object, so that seems fine.

[sirreal]

Thanks! I think there's one comment remaining to be addressed (#70466 (comment)) but otherwise this looks good.