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.

Sign up for GitHub

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

Jump to bottom

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1afcb56
fix(jest-mock): tweak typings to allow `jest.replace` replace methods
mrazauskas Mar 14, 2023
9151743
change log
mrazauskas Mar 14, 2023
eca27d5
clean up
mrazauskas Mar 14, 2023
f5edbed
rebase
mrazauskas Apr 3, 2023
641c21b
clean up
mrazauskas Apr 4, 2023
a8e8316
Merge branch 'main' into fix-jest-mock-replaceProperty-typings
mrazauskas Apr 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
- `[jest-config]` Handle frozen config object ([#14054](https://github.com/facebook/jest/pull/14054))
- `[jest-environment-jsdom, jest-environment-node]` Fix assignment of `customExportConditions` via `testEnvironmentOptions` when custom env subclass defines a default value ([#13989](https://github.com/facebook/jest/pull/13989))
- `[jest-matcher-utils]` Fix copying value of inherited getters ([#14007](https://github.com/facebook/jest/pull/14007))
- `[jest-mock]` Tweak typings to allow `jest.replaceProperty()` replace methods ([#14008](https://github.com/facebook/jest/pull/14008))
- `[jest-snapshot]` Fix a potential bug when not using prettier and improve performance ([#14036](https://github.com/facebook/jest/pull/14036))
- `[@jest/transform]` Do not instrument `.json` modules ([#14048](https://github.com/facebook/jest/pull/14048))

Expand Down
23 changes: 14 additions & 9 deletions packages/jest-mock/__typetests__/mock-functions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,20 +497,25 @@ expectError(

// replaceProperty + Replaced

const obj = {
fn: () => {},

const replaceObject = {
method: () => {},
property: 1,
};

expectType<Replaced<number>>(replaceProperty(obj, 'property', 1));
expectType<void>(replaceProperty(obj, 'property', 1).replaceValue(1).restore());
expectType<Replaced<number>>(replaceProperty(replaceObject, 'property', 1));
expectType<Replaced<() => void>>(
replaceProperty(replaceObject, 'method', () => {}),
);
expectType<void>(
replaceProperty(replaceObject, 'property', 1).replaceValue(1).restore(),
);

expectError(replaceProperty(obj, 'invalid', 1));
expectError(replaceProperty(obj, 'property', 'not a number'));
expectError(replaceProperty(obj, 'fn', () => {}));
expectError(replaceProperty(replaceObject, 'invalid', 1));
expectError(replaceProperty(replaceObject, 'property', 'not a number'));

expectError(replaceProperty(obj, 'property', 1).replaceValue('not a number'));
expectError(
replaceProperty(replaceObject, 'property', 1).replaceValue('not a number'),
);

interface ComplexObject {
numberOrUndefined: number | undefined;
Expand Down
24 changes: 10 additions & 14 deletions packages/jest-mock/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,17 +177,13 @@ export interface Replaced<T = unknown> {
* Restore property to its original value known at the time of mocking.
*/
restore(): void;

/**
* Change the value of the property.
*/
replaceValue(value: T): this;
}

type ReplacedPropertyRestorer<
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.

Just opened DefinitelyTyped/DefinitelyTyped#64936

(): void;
object: T;
property: K;
Expand Down Expand Up @@ -995,10 +991,10 @@ export class ModuleMocker {
/**
* Check whether the given property of an object has been already replaced.
*/
private _findReplacedProperty<
T extends object,
K extends PropertyLikeKeys<T>,
>(object: T, propertyKey: K): ReplacedPropertyRestorer<T, K> | undefined {
private _findReplacedProperty<T extends object, K extends keyof T>(
object: T,
propertyKey: K,
): ReplacedPropertyRestorer<T, K> | undefined {
for (const {restore} of this._spyState) {
if (
'object' in restore &&
Expand Down Expand Up @@ -1328,11 +1324,11 @@ export class ModuleMocker {
return descriptor[accessType] as Mock;
}

replaceProperty<
T extends object,
K extends PropertyLikeKeys<T>,
V extends T[K],
>(object: T, propertyKey: K, value: V): Replaced<T[K]> {
replaceProperty<T extends object, K extends keyof T>(
object: T,
propertyKey: K,
value: T[K],
): Replaced<T[K]> {
if (object === undefined || object == null) {
throw new Error(
`replaceProperty could not find an object on which to replace ${String(
Expand Down