Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(jest-mock): tweak typings to allow jest.replaceProperty() replace methods #14008

Merged
merged 6 commits into from
Apr 9, 2023
Merged

fix(jest-mock): tweak typings to allow jest.replaceProperty() replace methods #14008

merged 6 commits into from
Apr 9, 2023

Conversation

mrazauskas
Copy link
Contributor

@mrazauskas mrazauskas commented Mar 14, 2023

Reference DefinitelyTyped/DefinitelyTyped#64743

Summary

Current typing error, if a method is being replaced using jest.replaceProperty().

As it was pointed out in the above mentioned PR, jest.replaceProperty() is actually allowed to replace methods, not only properties. Even documentation is mentioning this use case (see last line):

Screenshot 2023-03-14 at 20 16 22

Test plan

Type test is added.

@mrazauskas mrazauskas changed the title fix(jest-mock): tweak typings to allow jest.replace() replace methods fix(jest-mock): tweak typings to allow jest.replaceProperty() replace methods Mar 14, 2023
T extends object,
K extends PropertyLikeKeys<T>,
> = {
type ReplacedPropertyRestorer<T extends object, K extends keyof T> = {
Copy link

Choose a reason for hiding this comment

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

I think that this should align with:

function replaceProperty<T, K extends keyof T>(obj: T, key: K, value: T[K]): ReplaceProperty<T[K]>;

in the types packages.

Currently (without this change) getting a typescript error:

node_modules/@jest/environment/build/index.d.ts:401:26 - error TS2430: Interface 'JestImportMeta' incorrectly extends interface 'ImportMeta'.
  The types of 'jest.replaceProperty' are incompatible between these types.
    Type '<T extends object, K extends Exclude<keyof T, keyof { [K in keyof T as Required<T>[K] extends ClassLike ? K : never]: T[K]; } | keyof { [K in keyof T as Required<T>[K] extends FunctionLike ? K : never]: T[K]; }>, V extends T[K]>(object: T, propertyKey: K, value: V) => Replaced<...>' is not assignable to type '<T, K extends keyof T>(obj: T, key: K, value: T[K]) => ReplaceProperty<T[K]>'.
      Types of parameters 'object' and 'obj' are incompatible.
        Type 'T' is not assignable to type 'object'.

401 export declare interface JestImportMeta extends ImportMeta {
                             ~~~~~~~~~~~~~~

  node_modules/@types/jest/index.d.ts:331:30
    331     function replaceProperty<T, K extends keyof T>(obj: T, key: K, value: T[K]): ReplaceProperty<T[K]>;
                                     ~
    This type parameter might need an `extends object` constraint.

So basically one of the issues is fixed here by not using PropertyLikeKeys anymore but T extends object is still incompatible with T so should likely also become just T here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm.. I think it would be better to have T extends object in @types/jest. It looks right to have a constrain here.

Copy link

Choose a reason for hiding this comment

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

As long as it's aligned I guess. But I don't think object is a good choice because object means "all non-primitive" types. So any class instances would also match that, for example. I don't know jest well enough to suggest what it should be but perhaps Record<.., ..> would be a better fit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If all non-primitive types are objects and if objects can have properties, I think it is alright to assume that these objects can have properties and that these properties might get replaced. Sometimes people add extra properties on arrays or functions. For instance, expect() has expect.not.stringContaining() which is the property of that function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SimenB
Copy link
Member

SimenB commented Apr 8, 2023

Can you merge in main so we can verify the changes on CI?

@mrazauskas
Copy link
Contributor Author

Done. All is green (;

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

thanks!

@SimenB SimenB merged commit c40ef21 into jestjs:main Apr 9, 2023
@mrazauskas mrazauskas deleted the fix-jest-mock-replaceProperty-typings branch April 9, 2023 08:54
@gausie
Copy link

gausie commented May 4, 2023

@SimenB is there a plan for releasing a 29.5.1 or something with this fix merged?

@github-actions
Copy link

github-actions bot commented Jun 4, 2023

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 4, 2023
@SimenB
Copy link
Member

SimenB commented Jul 4, 2023

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants