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

Vitest typings override instead of augmenting module #610

Closed
mlev opened this issue Jun 2, 2023 · 26 comments
Closed

Vitest typings override instead of augmenting module #610

mlev opened this issue Jun 2, 2023 · 26 comments

Comments

@mlev
Copy link

mlev commented Jun 2, 2023

Bug

The vitest typings still aren't quite right. The (currently unreleased) changes from PR #600 are an improvement but still not working for me. I believe this is because the vitest module typings are being overwritten instead of augmented so you end up with errors like:

TS2305: Module '"vitest"' has no exported member 'expect'.

Also - it's currently only applying the matchers as Asymmetric but it should be updating the Assertion interface as well (as per vitest doco https://vitest.dev/guide/extending-matchers.html)

I can get it working by patching the typings locally - and changing the vitest declaration to this:

import "vitest";
declare module "vitest" {
  // eslint-disable-next-line @typescript-eslint/no-empty-interface
  interface Assertion<T = any> extends CustomMatchers<T> {}
  // eslint-disable-next-line @typescript-eslint/no-empty-interface
  interface AsymmetricMatchersContaining extends CustomMatchers<any> {}
}

I believe you need the top level import to make it an augmentation. However - this requires importing vitest and I'm not sure what will happen when using jest. It might be fine but not 100% sure.

An alternative would be not to include this declaration and let users do the configuration themselves. Not as nice - but better than current solution which overwrites the vitest types.

I'm using Typescript 5.0.4 with NodeNext/ESM enabled.

  • package version: main branch
  • node version: 18.6.0
  • npm (or yarn) version: 9.5.1
@burtek
Copy link

burtek commented Jun 6, 2023

Same issue here with TS 5.1.3. jest-extended 4.0.0 just got released yesterday with this breaking change and types are broken for me

@burtek
Copy link

burtek commented Jun 6, 2023

Just FFR - my workaround is to use code as below when I need vitest types rather than jest-extend types

import type { SpyInstance } from 'vitest/dist/index';

This was referenced Jun 9, 2023
@GuillaumeRahbari
Copy link
Contributor

GuillaumeRahbari commented Jun 9, 2023

Sorry for the issue I introduced.

I made those 2 PRs

Both of them fixes the issue described by @mlev.

I do not have a perfect solution in mind. Each of the solution comes with advantages and drawbacks.

But in each of them I have trouble fixing the typescript errors

@GuillaumeRahbari
Copy link
Contributor

GuillaumeRahbari commented Jun 9, 2023

On my side I was using jest-extended only for this functionality https://jest-extended.jestcommunity.dev/docs/matchers/Array#toincludesamemembersmembers

But I found out that vitest has this natively with the chai api. So I will probably not be using jest-extended anymore

@Codex-
Copy link
Contributor

Codex- commented Jun 13, 2023

Also running into this issue, for now having to drop the jest-extended version 😔

Might need some input from @keeganwitt

@oskarols
Copy link

I think the preferable alternative here is to go with the PR that exports the types and leaves the Vitest configuration up to the user.

This also takes care of the case of using jest-extended together with your own additional matchers and supports using omit on the type in case not all of the matchers are in use.

@keeganwitt
Copy link
Collaborator

Also running into this issue, for now having to drop the jest-extended version 😔

Might need some input from @keeganwitt

I won't be able to get to it this week. But will try to next week

@AviDuda
Copy link

AviDuda commented Jul 3, 2023

I had to resolve it temporarily by applying a patch via patch-package.

File patches/jest-extended+4.0.0.patch:

diff --git a/node_modules/jest-extended/types/index.d.ts b/node_modules/jest-extended/types/index.d.ts
index 16626d3..30474c0 100644
--- a/node_modules/jest-extended/types/index.d.ts
+++ b/node_modules/jest-extended/types/index.d.ts
@@ -1,6 +1,11 @@
 /* eslint-disable @typescript-eslint/no-explicit-any */
 
-interface CustomMatchers<R> extends Record<string, any> {
+// jest-extended overrides the entire vitest module
+// see https://github.com/jest-community/jest-extended/issues/610
+// see types/vitest.d.ts
+export interface CustomMatchers<R> extends Record<string, any> {
   /**
    * Note: Currently unimplemented
    * Passing assertion

File types/vitest.d.ts:

import type { CustomMatchers } from "jest-extended";
import type { Assertion, AsymmetricMatchersContaining } from "vitest";

// jest-extended overrides the entire vitest module, so we need to re-extend it here
// see https://github.com/jest-community/jest-extended/issues/610

declare module "vitest" {
  interface Assertion<T = any> extends CustomMatchers<T> {}
  interface AsymmetricMatchersContaining extends CustomMatchers {}
}

@keeganwitt
Copy link
Collaborator

I had to resolve it temporarily by applying a patch via patch-package.

File patches/jest-extended+4.0.0.patch:

diff --git a/node_modules/jest-extended/types/index.d.ts b/node_modules/jest-extended/types/index.d.ts
index 16626d3..30474c0 100644
--- a/node_modules/jest-extended/types/index.d.ts
+++ b/node_modules/jest-extended/types/index.d.ts
@@ -1,6 +1,11 @@
 /* eslint-disable @typescript-eslint/no-explicit-any */
 
-interface CustomMatchers<R> extends Record<string, any> {
+// jest-extended overrides the entire vitest module
+// see https://github.com/jest-community/jest-extended/issues/610
+// see types/vitest.d.ts
+export interface CustomMatchers<R> extends Record<string, any> {
   /**
    * Note: Currently unimplemented
    * Passing assertion

File types/vitest.d.ts:

import type { CustomMatchers } from "jest-extended";
import type { Assertion, AsymmetricMatchersContaining } from "vitest";

// jest-extended overrides the entire vitest module, so we need to re-extend it here
// see https://github.com/jest-community/jest-extended/issues/610

declare module "vitest" {
  interface Assertion<T = any> extends CustomMatchers<T> {}
  interface AsymmetricMatchersContaining extends CustomMatchers {}
}

If we were to fix this the same way, this would introduce a new dependency on vitest, which I don't think we want to do.

@rluvaton
Copy link
Contributor

also noticed in x.each types broke, the parameters are not inferred anymore

@keeganwitt
Copy link
Collaborator

Do folks agree that reverting #600 would be a good at least temporary step?

@lukaselmer
Copy link
Contributor

lukaselmer commented Jul 14, 2023

Do folks agree that reverting #600 would be a good at least temporary step?

I agree. In the current state, jest-extended is unusable together with vitest.

However, #614 should solve this issue. Actually

import 'vitest';

is the most important part of this. If I add this line manually, everything starts working again

@Codex-
Copy link
Contributor

Codex- commented Jul 17, 2023

However, #614 should solve this issue. Actually

import 'vitest';

is the most important part of this. If I add this line manually, everything starts working again

Keegan addressed this specifically already, and why the pr is not ideal in it's current state: #610 (comment)

If we were to fix this the same way, this would introduce a new dependency on vitest, which I don't think we want to do.

@oskarols
Copy link

oskarols commented Jul 17, 2023

My vote is still for just exporting the types to the CustomMatchers, and providing documentation on how Vitest users can use these to add types themselves.

It is jest-extended afterall. I'm not expecting this package to go out of it's way to support Vitest; but providing an reasonable way of doing so would be much appreciated 👍

@mlev
Copy link
Author

mlev commented Jul 18, 2023

I agree. It's great we can still use this lib with vitest so a copy and paste of some typings to set it up is no problem.

@lukaselmer
Copy link
Contributor

However, #614 should solve this issue. Actually

import 'vitest';

is the most important part of this. If I add this line manually, everything starts working again

Keegan addressed this specifically already, and why the pr is not ideal in it's current state: #610 (comment)

If we were to fix this the same way, this would introduce a new dependency on vitest, which I don't think we want to do.

Yes, we would need to add vitest as a dev dependency. IMO that's OK, but I understand if you don't want that. However, that's actually not everything which would need to be changed in the typings 🙄 see main...lukaselmer:jest-extended:main

At the moment, it's an improvement either way (revert commit + docs, or add dev dependency and merge) 😉

@Codex-
Copy link
Contributor

Codex- commented Jul 19, 2023

Yes, we would need to add vitest as a dev dependency. IMO that's OK, but I understand if you don't want that.

The typings are part of the publishing package, and therefore the imports required for these to work correctly become real dependencies. Without this, consumers that only use jest and not vitest would be forced to use ts options to skip lib checking which is not good to impose on consumers.

So, IMO it's not OK, even though I am a vitest user.

keeganwitt added a commit that referenced this issue Jul 25, 2023
(rely on user provided types)
@keeganwitt
Copy link
Collaborator

I'm not a vitest user. Can someone validate the instructions in the PR I just opened?

keeganwitt added a commit that referenced this issue Jul 25, 2023
(rely on user provided types)
@pravinthan
Copy link

pravinthan commented Jul 26, 2023

Works for me using latest vitest and jest-extended! Appreciate the fix 💯

keeganwitt added a commit that referenced this issue Jul 26, 2023
(rely on user provided types)
keeganwitt added a commit that referenced this issue Jul 26, 2023
(rely on user provided types)
@keeganwitt
Copy link
Collaborator

This was the test I did with the fix, which seemed to work (though I'm no vitest expert)
https://github.com/keeganwitt/vitest-jest-extended-test

The only thing I'm not sure about is whether this part is actually needed

declare namespace jest {
  interface Matchers<R> {
    toHaveBeenCalledAfter(
      mock: jest.MockInstance<any, any[]> | import('vitest').MockInstance<any, any[]>,
      failIfNoFirstInvocation?: boolean,
    ): R;

    toHaveBeenCalledBefore(
      mock: jest.MockInstance<any, any[]> | import('vitest').MockInstance<any, any[]>,
      failIfNoSecondInvocation?: boolean,
    ): R;
  }
}

@pravinthan
Copy link

Actually, not sure what I'm doing wrong, but expect.toBeOneOf doesnt have a type. I'm using a global vitest setup, and I've followed the steps youve mentioned

@keeganwitt
Copy link
Collaborator

Actually, not sure what I'm doing wrong, but expect.toBeOneOf doesnt have a type. I'm using a global vitest setup, and I've followed the steps youve mentioned

Did you also add the setup below (already present in doc)?

import { expect } from 'vitest';
import * as matchers from 'jest-extended';

expect.extend(matchers);

keeganwitt added a commit that referenced this issue Jul 26, 2023
(rely on user provided types)
@pravinthan
Copy link

Yeah it's there

@keeganwitt
Copy link
Collaborator

Yeah it's there

Could there be some difference between our tsconfigs?

You could also try this instead

/// <reference types="vitest" />

export interface CustomMatchers<R> extends Record<string, any> {
  toHaveBeenCalledAfter(
      mock: jest.MockInstance<any, any[]> | import('vitest').MockInstance<any, any[]>,
      failIfNoFirstInvocation?: boolean,
  ): R;

  toHaveBeenCalledBefore(
      mock: jest.MockInstance<any, any[]> | import('vitest').MockInstance<any, any[]>,
      failIfNoSecondInvocation?: boolean,
  ): R;

  toHaveBeenCalledExactlyOnceWith(...args: unknown[]): R;
}

declare module 'vitest' {
  interface Assertion<T = any> extends CustomMatchers<T> { }
}

@keeganwitt
Copy link
Collaborator

Oh, I was also using jest-extended v3 to avoid the vitest changes made to the types file. That's another possible difference.

keeganwitt added a commit that referenced this issue Jul 26, 2023
(rely on user provided types)
@keeganwitt
Copy link
Collaborator

I'm gonna go ahead and merge this. We can follow-up with improving the documentation as a follow-up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
10 participants