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

Remove TypeScript warning from README #251

Merged
merged 1 commit into from Sep 23, 2020
Merged

Remove TypeScript warning from README #251

merged 1 commit into from Sep 23, 2020

Conversation

MoSattler
Copy link

This does not see to be the case anymore, TS works fine since #11

@codecov
Copy link

codecov bot commented May 13, 2020

Codecov Report

Merging #251 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #251   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           23        23           
  Lines          315       315           
  Branches        72        72           
=========================================
  Hits           315       315           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d6b60a9...377c600. Read the comment docs.

@gnapse
Copy link
Member

gnapse commented May 14, 2020

I'm not sure that this is the case.

When you import some matchers manually and not others, you need to extend expect yourself. And therefore, you need to provide the extend types for custom matchers yourself too, providing the types for only those that you manually imported and extended expect with.

Or am I missing something here?

@MoSattler
Copy link
Author

Can you give a concrete example where typing is not properly working ATM?

@gnapse
Copy link
Member

gnapse commented May 14, 2020

Ok, yes, you are right. Now I realized we have a different problem. If someone uses the custom matchers in this way, then they now have type definitions for all matchers, even those they did not import. They get auto-complete in jest-dom even. But then of course tests fail if they end up using the ones they did not import.

I wonder if we should remove this section altogether (the reference to importing custom matchers and extending expect manually). It does not seem much useful. I don't even know what the motivation would be to do so. It's not like these custom matchers are contributing to your bundle size or anything.

Would like to hear others' thoughts about this.

@MoSattler MoSattler changed the title Update README.md Remove TypeScript warning from README May 15, 2020
@MoSattler
Copy link
Author

@gnapse any news on this?

@gnapse
Copy link
Member

gnapse commented May 27, 2020

Oh yes, sorry. Would you mind removing the entire section altogether? From where we start saying "Alternatively you can import only the matchers you need" (something along those lines). It's line 121 I think.

If not don't worry, we can also merge this as is. Let me know.

@Grmiade
Copy link

Grmiade commented Jul 9, 2020

Typings is not properly working at the moment on my side with this example:

import {
  toBeInTheDocument,
  toHaveClass
} from "@testing-library/jest-dom/matchers";

expect.extend({ toBeInTheDocument, toHaveClass });
Could not find a declaration file for module '@testing-library/jest-dom/matchers'. '/Users/jeremy/Development/backoffice-web-client/node_modules/@testing-library/jest-dom/matchers.js' implicitly has an 'any' type.
  Try `npm install @types/testing-library__jest-dom` if it exists or add a new declaration (.d.ts) file containing `declare module '@testing-library/jest-dom/matchers';`

https://codesandbox.io/s/cocky-microservice-qwjx5?file=/src/setupTests.ts

@gnapse
Copy link
Member

gnapse commented Jul 10, 2020

The thing is we really do not support that and TypeScript at the same time. Not sure we can. My take on this was that we should remove this section altogether from the README. That's what I last requested from @MoSattler. Sadly I can't add changes to this PR, but I can start a new one superseding this one.

@MoSattler
Copy link
Author

@gnapse sorry for the delay. Did as you requested!

@gnapse gnapse merged commit f4c10c3 into testing-library:master Sep 23, 2020
@gnapse
Copy link
Member

gnapse commented Sep 23, 2020

@all-contributors add @MoSattler for docs

@allcontributors
Copy link
Contributor

@gnapse

I've put up a pull request to add @MoSattler! 🎉

@gnapse
Copy link
Member

gnapse commented Oct 23, 2020

🎉 This PR is included in version 5.11.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

None yet

3 participants