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

Move types from DefinitelyTyped #148

Merged

Conversation

TheGallery
Copy link
Collaborator

What:

Move types from DefinitelyTyped to this repository.

Why:

Reduce the amount of additional dependencies required and avoid issues like the one described here.

How:

Followed the approach taken in testing-library/react-testing-library#690 and testing-library/dom-testing-library#530

Checklist:

  • Documentation
  • Tests
  • Ready to be merged

The pre-commit hook was complaining about some type definition conflicts (as mentioned here) when running tsc so I had to skip it to create the PR. I could use some help resolving that if anyone becomes available.

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Great! Just one thing

package.json Outdated Show resolved Hide resolved
@kentcdodds
Copy link
Member

Should this be released as a breaking change? It technically is. I'm thinking I'd we do we could also remove the query* queries because Cypress doesn't need them thanks to the special behavior of the should('not.exist') assertion. Thoughts?

@TheGallery
Copy link
Collaborator Author

TheGallery commented Aug 18, 2020

Ah of course, they should be removed. I also agree this is a breaking change! I'll push this change later today.

package.json Outdated
"kcd-scripts": "^5.5.0",
"npm-run-all": "^4.1.5"
"npm-run-all": "^4.1.5",
"typescript": "^3.9.7"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added as a dependency because dtslint complains without it.

@kentcdodds kentcdodds changed the base branch from master to next August 22, 2020 22:39
@kentcdodds kentcdodds merged commit fdaa7e5 into testing-library:next Aug 22, 2020
@kentcdodds
Copy link
Member

I merged this into the next branch, but it should've been beta, so I'll do a little git gymnastics and then we'll have a beta release. Also, we'll want to make any other breaking change (like removing query*). Would you like to work on that?

@kentcdodds
Copy link
Member

Also, @TheGallery, thanks so much for your help! I've added you as a collaborator on the project. Please make sure that you review the other/MAINTAINING.md and CONTRIBUTING.md files (specifically the bit about the commit messages and the git hooks) and familiarize yourself with the code of conduct (we're using the contributor covenant). You might also want to watch the repo to be notified when someone files an issue/PR. Please continue to make PRs as you feel the need (you can make your branches directly on the repo rather than your fork if you want). Thanks! And welcome to the team :)

kentcdodds added a commit that referenced this pull request Aug 22, 2020
kentcdodds pushed a commit that referenced this pull request Aug 22, 2020
Co-authored-by: Kent C. Dodds <[email protected]>

BREAKING CHANGE: TypeScript type definitions have been brought into this module and no longer needs to be referenced from DefinitelyTyped
@TheGallery
Copy link
Collaborator Author

I merged this into the next branch, but it should've been beta, so I'll do a little git gymnastics and then we'll have a beta release. Also, we'll want to make any other breaking change (like removing query*). Would you like to work on that?

Gladly. Just to make sure, we want to remove the query* queries from the API completely instead of showing that error message?

@kentcdodds
Copy link
Member

The migration should just be a find and replace right? unless I'm missing something. If it's just a fine and replace, then I'm not worried about just removing it in a major version.

@TheGallery
Copy link
Collaborator Author

I am pretty sure it's me missing something. I am just confused on how removing query* queries compares to this #130. I am sorry about the confusion.

@kentcdodds
Copy link
Member

Oh yeah, I forgot about that. That didn't remove them, it deprecated them. Now we're need to remove them.

@TheGallery
Copy link
Collaborator Author

Right, I think I have a pretty good picture of what needs to be done now! I'll get to it. Thanks a bunch for the guidance.

@TheGallery TheGallery deleted the pr/migrate-ts-types-internally branch August 29, 2020 09:47
kentcdodds pushed a commit that referenced this pull request Sep 3, 2020
Co-authored-by: Kent C. Dodds <[email protected]>

BREAKING CHANGE: TypeScript type definitions have been brought into this module and no longer needs to be referenced from DefinitelyTyped
mvasin added a commit to mvasin/testing-library-docs that referenced this pull request Dec 27, 2020
superT999 added a commit to superT999/testing-library-docs that referenced this pull request Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants