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

createHydratedMock and cloneDeep from LoDash are not loving each other #751

Open
C0ZEN opened this issue May 22, 2021 · 9 comments
Open
Labels
enhancement New feature or request

Comments

@C0ZEN
Copy link
Contributor

C0ZEN commented May 22, 2021

Subject of the issue

After migrating from createMock to createHydratedMock, I had a failing test.
I added some debuggers and I found out that nothing happened after using a cloneDeep.
Removing the cloneDeep allow the test to pass as expected.
Using a spread operator is working as expected as well.
It means that there is something specific made inside the cloneDeep that createHydratedMock can't handle.

Your environment

  • ts-auto-mock version: 3.2.2
  • typescript version: 4.2.4
  • node version: 14.16.0
  • npm version: 7.12.1
  • lodash: 4.17.21

Steps to reproduce

This is the object that should be copied (as JSON):

{
  "response": "dummy-response",
  "options": {
    "tts": false,
    "nonce": "",
    "disableMentions": "none",
    "files": [],
    "code": "",
    "split": false,
    "content": "",
    "embed": {},
    "allowedMentions": {
      "parse": [],
      "roles": [],
      "users": []
    },
    "reply": {}
  }
}

image

Expected behavior

The test pass and the cloneDeep works as expected when using createHydratedMock.

Actual behavior

The test fail due to the cloneDeep.

Original issue

Sonia-corporation/sonia-discord#1383

@uittorio
Copy link
Member

Hi @C0ZEN we are getting there!!!

Do you have an isolated example of the issue?

Which object are you trying to clone and at which point?

I will try to use createHydratedMock in combination with cloneDeep but it will help having the specific example!

If you dont have time I'll try to find it from your repository!

@C0ZEN
Copy link
Contributor Author

C0ZEN commented May 23, 2021

@uittorio hello, I am reaching the end of my road to hydrated mocks ;)
You can see here https://github.com/Sonia-corporation/sonia-discord/pull/1383/checks?check_run_id=2647456167.
This is the PR with the migration and the failing tests are related to the cloneDeep usage.

@uittorio
Copy link
Member

So, I've done some research. The interface has recursion. CloneDeep will iterate through all the values of the interface it will reach a Maximum call stack size exceeded. If you wrap the clone deep code in a try and catch you'll see the error. Why is this happening with createHydratedMock and not createMock? Because the recursion is part of the not required interface.

@uittorio
Copy link
Member

uittorio commented May 23, 2021

I've created an example without ts auto mock

import { cloneDeep } from 'lodash';

interface Hello {
  a: Hello;
}
const createHello: () => Hello = () => {
  const m: Hello = {} as Hello;

  Object.defineProperties(m, {
    a: {
      get() {
        return createHello();
      },
      enumerable: true,
    },
  });

  return m;
};

it('should work', () => {
  const mock: Hello = createHello();

  const clonedType: Hello = cloneDeep(mock);

  expect(clonedType).toBeDefined();
});

image

@Pmyl what do you think?

@uittorio
Copy link
Member

uittorio commented May 23, 2021

If you instead use a class with a getter it's all good because the javascript converted has enumerable: false

class Hello {
  public get b(): Hello {
    return new Hello();
  }
  public test: string = '123';
}
var Hello = /** @class */ (function () {
    function Hello() {
        this.test = '123';
    }
    Object.defineProperty(Hello.prototype, "b", {
        get: function () {
            return new Hello();
        },
        enumerable: false,
        configurable: true
    });
    return Hello;
}());

@Pmyl
Copy link
Collaborator

Pmyl commented May 24, 2021

Yeah, cloneDeep for recursive interface would break it. It may be not break for createMock because it was defaulting some cases of Type | null to null so there is no recursion.

I guess it's dangerous to use cloneDeep on recursive interfaces in general, contracts like "this recursion is never infinite" cannot be defined in an interface, creating cases like ours where our implementation of that interface is correct but the code fails.
I checked and cloneDeep doesn't have any way to limit the depth, that would have been useful.
Maybe we have to create something ourselves to limit the max recursion depth? @uittorio

@uittorio
Copy link
Member

I think is a good idea to limit recursion, however recursive getter in classes works with cloneDeep! I was wondering if we are doing anything wrong!

@Pmyl
Copy link
Collaborator

Pmyl commented May 24, 2021

you're right, we should find a way to ensure it works the same way, sorry @C0ZEN this may take a bit :(

@C0ZEN
Copy link
Contributor Author

C0ZEN commented May 24, 2021

As usual guys, I am not rushing and this is an open-source project, so take the time you need and anyhow I have a fallback option with the spread operator (even though cloneDeep is better than spread in my XP).
Thanks!

@Pmyl Pmyl added the enhancement New feature or request label Jun 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants