fix: .mockRestore() should restore original implementation when calling getMockImplementation()

[kasperpeulen]

Description

We do some shady stuff in storybook, trying to automatically showing vitest spy in the storybook action panel:

https://github.com/storybookjs/storybook/blob/1d765656e8d3657baaf1b025c2571be48078171f/code/addons/actions/src/loaders.ts#L20-L29

  const previous = value.getMockImplementation();
  if (previous?._actionAttached !== true && previous?.isAction !== true) {
    const implementation = (...params: unknown[]) => {
      action(key)(...params);
      return previous?.(...params);
    };
    implementation._actionAttached = true;
    value.mockImplementation(implementation);
  }

However, this does not work after a spy is restored. As value.getMockImplementation() won't the original implementation, which I believe it should after restoring (not resetting) a mock. I think this is a bug in @vitest/spy.

There is a workaround though, that I implemented in this PR:
storybookjs/storybook#26364

But it seems weird that the getMockImplementation() value is out of sync with what is actually being "implemented" after restoring the spy. So feels like a bug to me, especially given the documentation:

  mockReset: () => this
  /**
   * Does what `mockReset` does and restores inner implementation to the original function.
   *
   * Note that restoring mock from `vi.fn()` will set implementation to an empty function that returns `undefined`. Restoring a `vi.fn(impl)` will restore implementation to `impl`.
   */

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
• Ideally, include a test that fails without this PR but passes with it.
• Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Tests

• Run the tests with pnpm test:ci.

Documentation

• If you introduce new functionality, document it. You can run documentation with pnpm run docs command.

Changesets

• Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

[netlify]

✅ Deploy Preview for fastidious-cascaron-4ded94 canceled.

Name	Link
🔨 Latest commit	5f2d8fb
🔍 Latest deploy log	https://app.netlify.com/sites/fastidious-cascaron-4ded94/deploys/65e9ac0ca17ea70008e5ec43

[sheremet-va]

Current behavior is correct. Restoring the mock should remove mockImplementation. This doesn't contradict the documentation since undefined implementation will fallback to the original function when the function is called. With the proposed change, getMockImlementation will always return a function which is not how it should work (according to the name) - calling restoreMock removes the mock, so it makes sense that getMockImplementation should return nothing.

[kasperpeulen]

@sheremet-va I don't follow your reasoning completely.

When I create a mock function with fn:

const mock = vi.fn(() => true)

Then initially the values are like this:

expect(mock()).toBe(true)
expect(mock.getMockImplementation()).toBeDefined()
expect(mock.getMockImplementation()?.()).toBe(true)

When I restore the mock, I would expect, the mock implementation to be the same as it was initially, right? But instead it is:

mock.restore();
expect(mock()).toBe(true)
expect(testFn.getMockImplementation()).toBe(undefined)

This seems at least asymmetric? I think I would get your reasoning, if initially, the implementation and the mock implementation would also be different:

const mock = vi.fn(() => true)
expect(mock()).toBe(true)
expect(testFn.getMockImplementation()).toBe(undefined)

But maybe that is just me.

If I follow your reasoning, would you agree that it would make sense then if we can get a mock.getImplementation method in the Mock interface. Because I don't see a way to get the current implementation, if there is no "mock" implementation set.

[kasperpeulen]

Anyway, will close this. And use mock[Symbol.for('tinyspy:spy')].impl for now to get the implementaiton.

[sheremet-va]

This seems at least asymmetric?

You have a point. But what we should do is the opposite - restore in this case should remove the () => true implementation. This will also make it jest-compatible. So it will behave like this:

mock.mockRestore();
expect(mock()).toBe(undefined)

I created an issue - #5436