testing-library / jest-dom

:owl: Custom jest matchers to test the state of the DOM
https://testing-library.com/docs/ecosystem-jest-dom
MIT License
4.45k stars 401 forks source link

fix: Standalone types for "./matchers" export and add Bun support #566

Closed jakeboone02 closed 10 months ago

jakeboone02 commented 10 months ago

What:

Fixes #554, type definitions for "./matchers" export.

Also adds support for Bun types when using bun:test.

Why:

This results in a TypeScript error:

import * as matchers from "@testing-library/jest-dom/matchers"
export.extend(matchers) // <-- matchers is not the correct type

Also...Bun is great.

How:

Referenced a new matchers-standalone.d.ts file in package.json#export#"./matchers" (not exported in the main export) that contains a version of matchers.d.ts converted to the correct types.

Checklist:

I don't think new documentation is necessary, but I'm not sure this is ready to be merged because I had to disable the husky config in order to commit. The part of the pre-commit script that failed was kcd-scripts test --findRelatedTests, I think because it saw a new file that didn't contain tests. But the new file doesn't really require JavaScript tests since it's a .d.ts file. Also the new Bun files have tests, so I'm not really sure what the husky config's problem is.

Edit 1: To clarify, this PR does not change the types for the main "." export. It only fixes the types for the "./matchers" export.

Edit 2: Looks like the husky/commit thing wasn't an issue and this is actually ready to be merged.

codecov[bot] commented 10 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (1fb156c) 100.00% compared to head (c17239d) 100.00%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #566 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 27 27 Lines 668 668 Branches 254 254 ========================================= Hits 668 668 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jakeboone02 commented 10 months ago

Yes, it makes sense because this new code is not replacing the matcher types in the main "." export. This PR only fixes the types for the "./matchers" export.

jakeboone02 commented 10 months ago

@gnapse Do you know if any other maintainers have looked at this? It's a relatively minor change, and as Bun (bun:test in particular) becomes more popular, this will avoid a lot of bug reports.

gnapse commented 10 months ago

It does not seem like they've seen it, but let's see if we can go ahead with this.

One last question before we merge: are you confident that this is a patch release? On one hand, I'm wondering if it should be a minor release, as it adds a new feature (bun support). OTOH, I wonder if it isn't a breaking change even. It does not look like it, but I wanted to make sure first.

jakeboone02 commented 10 months ago

It does not seem like they've seen it, but let's see if we can go ahead with this.

One last question before we merge: are you confident that this is a patch release? On one hand, I'm wondering if it should be a minor release, as it adds a new feature (bun support). OTOH, I wonder if it isn't a breaking change even. It does not look like it, but I wanted to make sure first.

I would say it's a judgement call whether this is a patch or minor. You could argue that the addition of Bun support is "fixing a bug specific to Bun," but it does seem like a new feature in a way, so strictly speaking a minor might be more appropriate per semver.

I'm all but certain this is not a breaking change. The only reason I'm not completely certain is there are just too many different environments and configurations in JS/TS land to be 100% certain of anything.

github-actions[bot] commented 10 months ago

:tada: This PR is included in version 6.2.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

axeleriksson147 commented 10 months ago

Hello @jakeboone02 and @gnapse

This breaks the TestingLibraryMatchers import for us in TypeScript. We have a file something like:

import { expect as _expect } from 'expect';
import { TestingLibraryMatchers } from '@testing-library/jest-dom/matchers';

declare module 'expect' {
  export interface Matchers<R = void> extends TestingLibraryMatchers<typeof _expect.stringContaining, R> {}
}

declare global {
  const expect: typeof _expect;
}

This way importing expect was including the extended TestingLibraryMatchers. With version 6.2.1 the import is incorrect.

jakeboone02 commented 10 months ago

@axeleriksson147 is the TestingLibraryMatchers type not available from the main "@testing-library/jest-dom" export? If you put a minimal repo together I'll take another look.

gnapse commented 10 months ago

Thanks for taking a look @jakeboone02. Please keep me in the loop if something actionable comes from it (e.g. if we have something to change and release to make things right again).

AxelEriksson0 commented 10 months ago

@gnapse and @jakeboone02 I created a minimal repo - https://github.com/AxelEriksson0/testing-library-jest-dom-6.2.0-breaking-type-location

My suggestion is to just export TestingLibraryMatchers in the matchers.standalone.ts.

jakeboone02 commented 10 months ago

@gnapse and @jakeboone02 I created a minimal repo - https://github.com/AxelEriksson0/testing-library-jest-dom-6.2.0-breaking-type-location

My suggestion is to just export TestingLibraryMatchers in the matchers.standalone.ts.

Thanks @AxelEriksson0! I patched the "broken" project with your suggestion and it seems to have done the trick.

If you think that resolves the issue I'll submit another PR.

axeleriksson147 commented 10 months ago

@gnapse and @jakeboone02 I created a minimal repo - https://github.com/AxelEriksson0/testing-library-jest-dom-6.2.0-breaking-type-location My suggestion is to just export TestingLibraryMatchers in the matchers.standalone.ts.

Thanks @AxelEriksson0! I patched the "broken" project with your suggestion and it seems to have done the trick.

If you think that resolves the issue I'll submit another PR.

Yeah, I think so. I just don't see TestingLibraryMatchers being exposed anymore, unless I'm missing something. But that should resolve it, thank you!

jakeboone02 commented 10 months ago

@AxelEriksson0 / @gnapse See #576.

paulleonartcalvo commented 6 months ago

@jakeboone02 Just came across this thread and am having what appears to be a unique issue. Im extending bun's expect with these matchers, but on error, some of the matchers like toBeInTheDocument cause errors related to the jest matcher utils like the following:

TypeError: this.utils.RECEIVED_COLOR is not a function. (In 'this.utils.RECEIVED_COLOR(this.isNot ? errorFound() : errorNotFound())', 'this.utils.RECEIVED_COLOR' is undefined)

Is this something that has been encountered or that a solution exists for?

jakeboone02 commented 6 months ago

@paulleonartcalvo can you provide a reproduction?

paulleonartcalvo commented 6 months ago

@jakeboone02 👍 I'll try and set up an environment tonight. For awareness, i was using Bun + typescript + React + Vite