Isn't name given to 'instances' property of 'mockFn.mock' is little misleading?

[segmentationfaulter]

Suppose we have a mock constructor which we can use to create instances of Car like this:

const Car = jest.fn()
const car = new Car()
Car.mock.instances[0] === car

Snippet of code above makes sense to me, but what you will encounter below can mislead some developers

const foo = {}
const bar = Car.bind(foo)
bar()
Car.mock.instances[1] === foo

Since foo is not instance of Car, it was just bound to be this context upon invoking bar as a regular function, so pushing it to Car.mock.instances does not make much sense. Shouldn't instances property be better named/aliased as contextObjects to get rid of this confusion?

[SimenB]

I agree it's confusing, but I'm not sure what the best solution is...

/cc @thymikee @rickhanlonii @UselessPickles

[rickhanlonii]

I don't think the issue is the binding, I think the issue is pushing to instances when not instantiating

For example, this test fails (notice that instances is [undefined]):

test('no instances', () => {
  const Car = jest.fn();
  const car = Car(); // Note no "new" keyword
  expect(Car.mock.instances).toHaveLength(0);
});

When you do this with the bind and foo in OP, you get instances as [{}]

test('no instances', () => {
  const Car = jest.fn();
  const foo = {};
  const bar = Car.bind(foo);
  bar();
  expect(Car.mock.instances).toHaveLength(0);
});

[UselessPickles]

@rickhanlonii Are you saying that nothing should be pushed onto the instances array when the mock is called without instantiating?

The current design of how all the call information for mocks is maintained is with parallel arrays, where index 1 (for example) in all the arrays is expected to contain the relevant information for the second call to the mock. Based on this design, pushing undefined onto the instances array is the correct behavior when a call is made without instantiating. If nothing is pushed onto the array, then the parallel arrays fall out of sync with each other.

It would be nice to combine all of these parallel arrays into a single array of objects where each entry in the one array contains ALL information about how the mock was called, how it completed, what it returned/threw, etc.

[UselessPickles]

According to this, it is possible to detect whether a function was called as a constructor as of ECMAScript 6: https://stackoverflow.com/questions/367768/how-to-detect-if-a-function-is-called-as-constructor/31060154#31060154

new.target is the key: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/new.target

I don't quite understand all possible environments that Jest can execute within. Can we assume that new.target is supported in all environments? If so, it would be quite simple to fix this bug so that non-undefined entries exist in instances only for calls that were legitimately used as constructors.

[SimenB]

jest-mock can run in the browser, so it would be nice to support IE11. No biggie, though. Can we feature detect it?

[UselessPickles]

Another thing to consider: if a function used as a constructor returns an object, then that return value is returned by the new expression, instead of returning the new this instance (can be used to implement caching for a "multiton" pattern in a way that is transparent to users of the class/constructor, for example).

In that situation, do we still want to store this in the instances array? Is it misleading to indicate that an instance was created, even though the instance was not returned by the new expression? What exactly are we trying to capture? That the function was called like a constructor with the new keyword? Or that a new instance was obtained by the calling code? Should we be storing more information to handle this edge case?

[SimenB]

What exactly are we trying to capture?

I'm not sure to be honest 😅 It's been there since the "FIRST" commit back in 2014, but I can't find it in Jasmine's docs. I'd happily remove it since I've never had any use for it whatsoever, but that might just be me. What use-cases are covered by instances but not calls?

[UselessPickles]

Can we feature detect it?

Since new is a keyword rather than an a global object, new.target might be a syntax error in IE11, rather than something that simply resolves to undefined or causes a run-time error that could be wrapped in a try/catch for feature detection.

If that's the case, maybe it could be wrapped in eval() to move the syntax error into the eval execution so that it can be caught at run-time? But my gut feeling is that eval("new.target") might not produce the expected result in environments that DO support new.target, just because it's such a special thing that is sensitive to being referenced from within a constructor call, and eval is weird about the context it executes within in ways that I don't understand.

Short version: It probably requires some experimentation, but I wouldn't be surprised if it was impossible to both use new.target and have code that doesn't blow up in IE11.

[UselessPickles]

What use-cases are covered by instances but not calls?

I don't see how calls could possibly cover any use case that is covered by instances, or vice-versa. calls only contains the an array of the arguments to the function call, while instances is intended (I assume, based on its name...) to store a reference to the instance that was created by calling the function as a constructor with the new keyword.

One simple possibility is to rename/document instances to contexts or something similar to match its current implementation: it simply stores the this context upon which the call was executed, and makes no promises about whether this means it was called like a constructor or not.

[SimenB]

I meant results, not calls, sorry!

[UselessPickles]

Still, there's no overlap between results and instances. results contains info about what value was returned/thrown by the function. instances contains the this execution context (which may be the newly created instance if the function was called via new, and the function did not explicitly return anything).

Aside from the (probably rare) edge case I mentioned a few comments ago about implementing a multiton pattern in the constructor, constructor functions themselves don't return anything. The new expression creates a new object, executes the constructor on that new object, then returns the new object.

[github-actions]

This issue is stale because it has been open for 1 year with no activity. Remove stale label or comment or this will be closed in 14 days.

[github-actions]

This issue was closed because it has been stalled for 7 days with no activity. Please open a new issue if the issue is still relevant, linking to this one.

[github-actions]

This issue 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.