Skip to content

TypeScript: migrate jest-console package to TS - #70538

Merged
manzoorwanijk merged 10 commits into
WordPress:trunkfrom
kushagra-goyal-14:try/migrate-jest-console-ts
Jul 24, 2025
Merged

TypeScript: migrate jest-console package to TS#70538
manzoorwanijk merged 10 commits into
WordPress:trunkfrom
kushagra-goyal-14:try/migrate-jest-console-ts

Conversation

@kushagra-goyal-14

Copy link
Copy Markdown
Contributor

What?

Part of #67691

Migrating the jest-console package to TypeScript.

Why?

To enhance DX and add type safety.

How?

By porting the code and tests to TypeScript.

Testing Instructions

Typecheck and unit tests.

@t-hamano t-hamano mentioned this pull request Jun 25, 2025
41 tasks
@t-hamano t-hamano added [Type] Code Quality Issues or PRs that relate to code quality [Tool] Jest console /packages/jest-console labels Jun 27, 2025
kushagra-goyal-14 and others added 2 commits June 27, 2025 19:24
@kushagra-goyal-14
kushagra-goyal-14 marked this pull request as ready for review June 27, 2025 14:15
@github-actions

github-actions Bot commented Jun 27, 2025

Copy link
Copy Markdown

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: kushagra-goyal-14 <kush123@git.wordpress.org>
Co-authored-by: manzoorwanijk <manzoorwanijk@git.wordpress.org>

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

Comment on lines +61 to +69
expect( ( spy as any ).assertionsNumber ).toBe( 0 );

console[ methodName ]( message );

expect( console )[ matcherName ]();
expect( spy.assertionsNumber ).toBe( 1 );
expect( ( spy as any ).assertionsNumber ).toBe( 1 );

expect( console )[ matcherNameWith ]( message );
expect( spy.assertionsNumber ).toBe( 2 );
expect( ( spy as any ).assertionsNumber ).toBe( 2 );

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need these any assertions here? I think those can be removed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have removed those unnecessary types from the tests.

@kushagra-goyal-14

Copy link
Copy Markdown
Contributor Author

Hey @manzoorwanijk ,

Thank you for the review.

I have updated the PR with the changes. Can you please re-review or ship the PR whenever possible.

@manzoorwanijk manzoorwanijk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still see some as any assertions but I am not sure if we can entirely get rid of all of those.

@kushagra-goyal-14

kushagra-goyal-14 commented Jul 10, 2025

Copy link
Copy Markdown
Contributor Author

I still see some as any assertions but I am not sure if we can entirely get rid of all of those.

Here, there are some cases with usage of as any, because the property assertionsNumber doesn't exist for the jest spy definition, thus I have used as any there.

@manzoorwanijk manzoorwanijk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unknown is a safer type than any because it narrows the scope instead of expanding it.

So, let us use unknown where we don't really know the type.

We can also improve some any types with some properly defined types.

Comment thread packages/jest-console/src/matchers.ts Outdated
Comment on lines +15 to +18
calls: any[][];
matcherName: string;
methodName: string;
expected?: any[];

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
calls: any[][];
matcherName: string;
methodName: string;
expected?: any[];
calls: unknown[][];
matcherName: string;
methodName: string;
expected?: unknown[];

Comment thread packages/jest-console/src/matchers.ts Outdated
spy: Mock,
matcherName: string,
methodName: string,
expected?: any[]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
expected?: any[]
expected?: unknown[]

Comment thread packages/jest-console/src/matchers.ts Outdated
Comment on lines +69 to +70
( matcherName: string, methodName: string ) => ( received: any ) => {
const spy = received[ methodName ] as Mock;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can get rid of any type and the assertion

Suggested change
( matcherName: string, methodName: string ) => ( received: any ) => {
const spy = received[ methodName ] as Mock;
( matcherName: string, methodName: string ) =>
( received: Record< string, Mock > ) => {
const spy = received[ methodName ];

Comment thread packages/jest-console/src/matchers.ts Outdated
Comment on lines +80 to +81
function ( received: any, ...expected: any[] ) {
const spy = received[ methodName ] as Mock;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
function ( received: any, ...expected: any[] ) {
const spy = received[ methodName ] as Mock;
function ( received: Record< string, Mock >, ...expected: unknown[] ) {
const spy = received[ methodName ];

Comment thread packages/jest-console/src/matchers.ts Outdated
};
},
{}
{} as Record< string, any >

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this assertion here, I would pass this as the first argument to th .reduce generic

Record<
	string,
	( received: Record< string, Mock > ) => {
		pass: boolean;
		message: () => string;
	}
>

So, the .reduce call would look like this

Object.entries( supportedMatchers ).reduce<
	Record<
		string,
		( received: Record< string, Mock > ) => {
			pass: boolean;
			message: () => string;
		}
	>
>(/* Arguments here */);

@kushagra-goyal-14

kushagra-goyal-14 commented Jul 11, 2025

Copy link
Copy Markdown
Contributor Author

I've implemented the suggested changes to improve type safety in the jest-console package. Created a dedicated types.ts file with proper interfaces, replaced any with unknown where appropriate, and used more specific types throughout the codebase.

cc: @manzoorwanijk

@manzoorwanijk manzoorwanijk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to update the types field in package.json for the package to build-types but also handle the existing types/index.d.ts, which IMHO can be changed to declarations.d.ts and included via files field in tsconfig.json.

@manzoorwanijk manzoorwanijk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect. Thanks

@manzoorwanijk
manzoorwanijk merged commit 8227e4b into WordPress:trunk Jul 24, 2025
61 checks passed
@github-actions github-actions Bot added this to the Gutenberg 21.4 milestone Jul 24, 2025
adamsilverstein pushed a commit to adamsilverstein/gutenberg that referenced this pull request Jul 31, 2025
Co-authored-by: kushagra-goyal-14 <kush123@git.wordpress.org>
Co-authored-by: manzoorwanijk <manzoorwanijk@git.wordpress.org>
@aduth

aduth commented Aug 14, 2025

Copy link
Copy Markdown
Member

I wonder if this might have regressed the availability of typings for Jest matchers in downstream projects that use this package?

In @wordpress/jest-console@8.26.0 (before this change), I could configure TypeScript compilerOptions.types to include "@wordpress/jest-console", and get types for the matcher:

image

With the latest version (8.28.0) including these changes, the types are no longer found:

image

I suspect it may be related to the change here to move the Jest Matchers types originally in types/index.d.ts to src/declarations.d.ts. As far as I can tell, these declarations are no longer available in the published package.

Compare types root between 8.26.0 and 8.28.0:

Edit: Trying to explore a fix at #71215

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Tool] Jest console /packages/jest-console [Type] Code Quality Issues or PRs that relate to code quality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants