Skip to content

Conversation

dariye
Copy link
Contributor

@dariye dariye commented Sep 16, 2019

Update installation instruction for Typescript

What:

  • Documentation

Why:

  • Update instruction for Typescript

How:

Checklist:

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

Update installation instruction for Typescript
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.

Thanks for your contribution. Indeed, there are things to be improved about this TypeScript aspect in the README. However, I'm not entirely sure this replacement is accurate with respect to the intentions of the text you're replacing.

The text being replaced was meant to convey the fact that picking and choosing to import just some of the custom matchers, and extent jest just with the ones you want, and not all, this won't make TypeScript happy. And using a setup file in TS as you mention won't fix that either.

Using a TS setup file will fix importing jest-dom in the usual manner. So I think the text you have included here, rather than replacing this part of the README, should be inserted, without replacing anything, before the paragraph in the README a few lines above this part, the one that starts with the sentence "Alternatively, you can selectively import only..."

Can you comment on these concerns of mine and clarify if I'm misreading something here?

(Also, as a side note, this PR is related to #123 so I'll mention that here to create the cross reference to that issue, which, when solved, may even change this update).

@dariye
Copy link
Contributor Author

dariye commented Sep 17, 2019

Thanks for your contribution. Indeed, there are things to be improved about this TypeScript aspect in the README. However, I'm not entirely sure this replacement is accurate with respect to the intentions of the text you're replacing.

The text being replaced was meant to convey the fact that picking and choosing to import just some of the custom matchers, and extent jest just with the ones you want, and not all, this won't make TypeScript happy. And using a setup file in TS as you mention won't fix that either.

Using a TS setup file will fix importing jest-dom in the usual manner. So I think the text you have included here, rather than replacing this part of the README, should be inserted, without replacing anything, before the paragraph in the README a few lines above this part, the one that starts with the sentence "Alternatively, you can selectively import only..."

Can you comment on these concerns of mine and clarify if I'm misreading something here?

(Also, as a side note, this PR is related to #123 so I'll mention that here to create the cross reference to that issue, which, when solved, may even change this update).

Hey @gnapse, I think you're right. I misread that. I'll put back that paragraph.

dariye and others added 2 commits September 17, 2019 10:12
Co-Authored-By: Ernesto García <gnapse@gmail.com>
@dariye
Copy link
Contributor Author

dariye commented Sep 17, 2019

I've updated per your comment and suggestion @gnapse and thanks!

@dariye dariye requested a review from gnapse September 17, 2019 08:15
@gnapse gnapse merged commit a204bae into testing-library:master Sep 17, 2019
@gnapse
Copy link
Member

gnapse commented Oct 7, 2019

🎉 This PR is included in version 4.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@gnapse gnapse added the released label Oct 7, 2019
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.

2 participants