Document the fact that arguments to mocked functions are not cloned

[dbstraight]

This caught me out and took me a while to debug. Because tinyspy doesn't clone the arguments when tracking calls (https://github.com/tinylibs/tinyspy/blob/cd4d86683a2a62f34d9df90308039d1faa88f9f7/src/internal.ts#L83), you can get quite unexpected results if you are calling a mocked function multiple times while mutating the object it is being called with. The .mock.calls property will only show the final value of the object passed to the mock function because the logged calls in tinyspy get mutated out from under it.

I'd suggest documenting here: https://vitest.dev/guide/mocking.html#mocking-pitfalls

And an example code could be included showing how you could create your own local list of calls in a mockImplementation if you needed to be able to handle this situation.

[hi-ogawa]

you can get quite unexpected results if you are calling a mocked function multiple times while mutating the object it is being called with.

I totally get that this kind of mutability issue appears very confusing and hard to track down, but I'm not actually sure if cloning arguments is "correct" either in this context.

Not only about what/how to clone and its perf impact, but also, for example, I feel it's also natural for users to wish to assert some arguments of calls are identically equal:

const fn = vi.fn();
const something = {};
fn(something);
fn.mock.calls[0][0] === something; // it would be false if deep cloned

So, this looks to me as a more general pitfalls of writing mutable code and not necessary about mocking or Vitest. Actually you didn't provide any code, so I'm just guessing your situation. If I misunderstood the issue, I would appreciate if you can provide more concrete code to illustrate your concern.

Also do you know what other mocking library such as sinon or jest does? If there is a difference, then that can be also a good guidance about what to do.

[hi-ogawa]

Wow, that looks very familiar. I think I really had the similar situation like yours before as I also tend to just dump everything by expect(someFn.mock.calls).toMatchInlineSnapshot().

I still think this is a general programming issue and personally I don't know where is a right place to put this note. But please feel free to send a PR where you want to update the doc.

[sheremet-va]

Personally, I think this is not a Vitest pitfall, this is how JavaScript works, and I don't see any reason to put it anywhere in our documentation.

[dbstraight]

The only way I was able to know that arguments were not cloned was to see it happen and then confirm by going into tinyspy source. That's a design decision which is not necessarily obvious for users of vitest. I'm not suggesting to document how mutating JS objects works. I'm suggesting to document that a design decision was made (maybe by tinyspy not vitest, but somewhere along the way) not to clone arguments when keeping track of mocks. There is no way a user can just know that, regardless how much they know about how JS works.

[sheremet-va]

From my point of view, there is nothing to suggest that objects should or could be cloned. If you are working with JavaScript, you assume object values share a reference. tinyspy didn't make any decisions on how it works, it just follows default JS behavior. If we start explaining how objects work, we would have to explain other JS concepts that are better suited on MDN.

[dbstraight]

So, to wrap this up, in my mind it's something like logging.

If you call a logging library with an object, then mutate the object, you're not expecting the mutated object to be in the logs, no matter what buffering or internal operations the logging library might do with that object before writing it to the logs.

In any event, the existence of this discussion thread basically serves the same purpose of the docs update I'm proposing... it gives future users something to find that explains mocking and cloning.

I respect the decision despite seeing it differently. Thanks for the dialogue on it! 😃