Fix DeepMockProxy type error for objects which are functions and have fields at the same time

[lallenfrancisl]

For some types like AxiosInstance the object (or rather better called as a function that has fields) doubles as function. This means that in the current type implementation for DeepMockProxy the if condition gets triggered for the function part. This causes the type to incorrectly say that only the first nested field is possible. To show this in action I am putting a simplified version similar to how AxiosInstance behaves when used inside a class. You can find the original implementation of AxiosInstance here

import { CalledWithMock, DeepMockProxy } from './Mock';

interface DemoInter {
    (config: Record<string, any>): Promise<any>;
    (url: string, config?: Record<string, any>): Promise<any>;

    request(inp: string): Promise<any>;

    dataField: {
        hello: 'world';
    };
}

class DemoClass {
    client: DemoInter;

    constructor(a: DemoInter) {
        this.client = a;
    }
}

type OldDeepMockProxy<T> = {
    // This supports deep mocks in the else branch
    [K in keyof T]: T[K] extends (...args: infer A) => infer B ? CalledWithMock<B, A> : OldDeepMockProxy<T[K]>;
} & T;

let oldMock: OldDeepMockProxy<DemoClass>;

// This would fail compilation with existing DeepMockProxy
oldMock.client.request.mockImplementationOnce();

let newMock: DeepMockProxy<DemoClass>;

// This is properly typed
newMock.client.request.mockImplementationOnce();

// Properly works for other data fields
newMock.client.dataField.hello;

This PR fixes #94

[lallenfrancisl]

@marchaos I know its kind of annoying to tag, but doing just in case this does not get lost in your notifications

[marchaos]

Thanks. This did actually get lost in my notifications! Any chance you can add a test? That will help me understand the issue a bit more too.

[lallenfrancisl]

Yeah working on some tests, at first I thought since this is type change tests wouldn't be needed. But this is something that needs to be tested to know if it works too

[lallenfrancisl]

This test and the test after this are the ones that test the new behaviour

[lallenfrancisl]

We have to call DeepMockProxy in the case of functions too because some libraries (like Axios) use the functions to have other nested functions since its valid in javascript to to that

[lallenfrancisl]

@marchaos I have added some unit tests. Some of the tests are actually duplicated from the mockDeep's existing tests, but changed to test everything works here too. I have added comments in the file to highlight this

[marchaos]

Sweet thanks!, taking a look...

[marchaos]

I pushed a new version 2.0.7 with this fix. Let me know if you've got any issues with that.

Thanks for the PR

[marchaos]

Worth noting that this has introduced a performance issue, which is now resolved in the 3.0.x branch. This required an implementation change so that functions with prop support is now opt in. see issue #97 for more details. cc @lallenfrancisl

I haven't pushed a 3.0.0 final version yet, this is just a heads up.

[lallenfrancisl]

Dang sorry, didn't expect that. I can also look into some alternative while avoiding performance issue. Will try to sit on this @marchaos

[marchaos]

Hey @lallenfrancisl , I've already got an alternative for this:

mockDeep({ funcPropSupport : true })

This means that you only need to pass that in if you want to support functions with props / fields. For 99% of cases / people, this won't be required for deep mocking, but didn't want to remove that functionality. This is also why there will be a major version upgrade (3.0.0). If you want to test it out before 3.0.0 is released, you can use 3.0.0-beta1.

[lallenfrancisl]

I saw the fix, but this still causes the issue when using the functionality, right ? I am not sure if I will get the time to test out this week. When are you planning to release 3.0 ?

[marchaos]

Yes it does. Would be nice to get a perf fix for that, but inherently it's recursive in both branches so will be difficult to fix.

…

On Tue, 13 Sep 2022, 17:30 Allen Francis, ***@***.***> wrote: I saw the fix, but this still causes the issue when using the functionality, right ? I am not sure if I will get the time to test out this week. When are you planning to release 3.0 ? — Reply to this email directly, view it on GitHub <#95 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAVBY6OA5VWZUYXYELVEXD3V6CT3DANCNFSM53BRJ4KA> . You are receiving this because you were mentioned.Message ID: ***@***.***>