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

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

Merged
merged 2 commits into from
Jan 22, 2024

Conversation

jakeboone02
Copy link
Contributor

@jakeboone02 jakeboone02 commented Jan 4, 2024

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:

  • Documentation
  • Tests
  • Updated Type Definitions
  • Ready to be merged

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.

Copy link

codecov bot commented Jan 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@gnapse gnapse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it looks good, but I'll wait for one or two other reviews from our maintainers before merging.

Also, does it make sense to keep matchers.d.ts if you are replacing its export with the new matchers-standalone.d.ts file?

@jakeboone02
Copy link
Contributor Author

jakeboone02 commented Jan 4, 2024

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
Copy link
Contributor Author

@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
Copy link
Member

gnapse commented Jan 22, 2024

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
Copy link
Contributor Author

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.

@gnapse gnapse merged commit 5675b86 into testing-library:main Jan 22, 2024
7 checks passed
Copy link

🎉 This PR is included in version 6.2.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@jakeboone02 jakeboone02 deleted the pr/matchers-standalone branch January 22, 2024 19:15
@axeleriksson147
Copy link

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
Copy link
Contributor Author

@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
Copy link
Member

gnapse commented Jan 30, 2024

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
Copy link

@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
Copy link
Contributor Author

@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
Copy link

@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
Copy link
Contributor Author

@AxelEriksson0 / @gnapse See #576.

@paulleonartcalvo
Copy link

@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
Copy link
Contributor Author

@paulleonartcalvo can you provide a reproduction?

@paulleonartcalvo
Copy link

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

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

Successfully merging this pull request may close these issues.

TypeScript types for the "./matchers" export don't match implementations
5 participants