The new.target is undefined even if spy is created with new keyword

[trivikr]

Describe the bug

The new.target is undefined if spy is created with new keyword.

From MDN for new.target

The new.target meta-property lets you detect whether a function or constructor was called using the new operator. In constructors and functions invoked using the new operator, new.target returns a reference to the constructor or function that new was called upon. In normal function calls, new.target is undefined.

Steps to reproduce

const fn = {
  fnFunc: () => {},
  fnClass: class FnClass {},
};

spyOn(fn, "fnFunc").willCall(function () {
  console.log(`Called using ${new.target ? "new operator" : "function call"}`);
});
spyOn(fn, "fnClass").willCall(function () {
  console.log(`Called using ${new.target ? "new operator" : "function call"}`);
  return fn.fnClass;
});

fn.fnFunc(); // Called using function call
new fn.fnClass(); // Called using function call

Observed behavior

Called using function call
Called using function call

Expected behavior

Called using function call
Called using new operator

Additional context

Noticed while debugging an issue in vitest at vitest-dev/vitest#2821 (comment)

[trivikr]

Attempted a fix in #30

[sheremet-va]

I was also working on it in #31 😄

[trivikr]

I was also working on it in #31 😄

Well! Works as long as it's fixed 😄

[trivikr]

Adding a summary from #32

Verified that the following solution works:

        result = new.target
          ? new (fn.impl as any)(...args)
          : fn.impl.apply(this, args)
        if (new.target) Object.setPrototypeOf(r, this);

Since MDN recommends against Object.setPrototypeOf because of performance impact, @sheremet-va questioned whether this should actually be considered a bug.

It exists in sinon and jest-mock, other mock/spy utilities.

[sheremet-va]

We can add somewhere how it was called, for example. Store constructor/call string with calls.

[trivikr]

whether this should actually be considered a bug.

This issue is a bug as per the definition of new.target.

Now tinyspy/vitest needs to decide whether they want to fix this bug, or want to retain this bug since it's present in sinon/jest-mock.

If I was a maintainer, I would have chosen to fix it and either provide a documentation or transformation in a tool (like vitest-codemod) to fix this in users codebase while they migrate.

For example, vitest provides documentation for the following things which are not supported:

• Module mocks for default export in https://vitest.dev/guide/migration.html
• Lack of support for requireActual in Migrate mocked useNavigate hook that uses jest.requireActual vitest-dev/vitest#940 (comment)

I've added transformation in vitest-codemod for the following things which are not supported:

• Replace jest object with vi in https://github.com/trivikr/vitest-codemod/blob/main/packages/jest/src/apis/replaceJestObjectWithVi.ts
• Replace test API failing to fails in https://github.com/trivikr/vitest-codemod/blob/main/packages/jest/src/apis/replaceTestApiFailing.ts
• Replace fit API to it.only in https://github.com/trivikr/vitest-codemod/blob/main/packages/jest/src/apis/replaceTestApiFit.ts

[trivikr]

We can add somewhere how it was called, for example. Store constructor/call string with calls.

This will fix the issue in vitest, and not in tinyspy.

That works for me as a user of vitest.

[sheremet-va]

This issue is a bug as per the definition of new.target.

It cannot be a bug by definition, because custom implementation is always „called“, not „constructed“. Because it is tinyspy who calls it, and not a user. And tinyspy has information if it was called with new.

[trivikr]

And tinyspy has information if it was called with new.

Since tinyspy is a spy library, should it imitate what user calls?
i.e. Imitate new.target being undefined as per the definition when new operator is used on spied module.

[trivikr]

We can add somewhere how it was called, for example. Store constructor/call string with calls.

How about adding the instances and contexts members in tinyspy itself?
It already keeps track of calls and results which vitest re-exports.

Something like this:

try {
  result = fn.impl.apply(this, args)
  if (new.target)
    fn.instances.push(this)
  type = 'ok'
}

// ...

const reset = () => {
  // ...
  fn.instances = []
}

And vitest could just export values from stub in it's spy implementation

  const mockContext = {
    get calls() {
      return stub.calls
    },
    get instances() {
      return stub.instances
    },

This will need v2.0.0 release for tinyspy though, to maintain backward compatibility.