-
Notifications
You must be signed in to change notification settings - Fork 232
Convert to TypeScript #498
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
Comments
The compiler won't recommend looking for Is there a reason you think we should |
There's a list of removed packages that you can submit a pull request to. |
Only that I have seen folks on Twitter mention how nice it was to use. I'm also happy with kcd-scripts if that is the preferred approach. |
If you want I can migrate the actual codebase to kcd-scripts. |
In that case it's probably easier to do #499 with |
Yes, sure |
@marcosvega91 @mpeyper I'd love to help when it comes to migrating to TypeScript 💕 |
You can start building off the changes in #502 which I'll merge as soon as I'm on my laptop next (I need to update some repo settings first) |
@mpeyper Do you mean building in the same PR as @marcosvega91? Then he would have to give me permission as a collaborator to be able to push changes there, or am I missing something? I could start building once that PR is merged 🙌 🥰 Thanks for the opportunity also 🙏 💕 |
No, I mean pull down his PR branch, branch from there, and start committing onto it. Then rebase to master once I've merged it (within 24 hours). |
@mpeyper How do I do that? I have forked render hooks and my current branch is master. How do I pull down his PR branch? Thanks! 🙏 |
I'm only on my phone, so I'm a bit hamstrung to give you the exact commands, but first you need to add their repo as a remote, then you pull the changes from that remote's branch. You can follow the links near the top of the PR page to work out the user and branch names involved. |
You can also use the |
Will do it later, two tasks to cross off my side project, I am looking forward to it though haha 😍 💯 Probably just install Thank you guys, you are so awesome 😄 |
#502 has been merged, so at this point you can pull down master and branch off it directly. |
Yeah lit 🔥 🤘, looking forward to it 😄 |
@nickmccurdy @mpeyper @marcosvega91 I could create the first PR with a small change, or how do you want to have it? |
This package has a relatively simple API so it might be easier to review in a single PR that converts everything, but it's up to you. |
I was thinking of other's wanted to collaborate, how would we do it then? One option is to invite someone as a collaborator to my fork of render hooks right? |
If you open a pull request, maintainers should be able to push to your fork automatically, and you can invite anyone else. If permissions become an issue we might be able to open a branch here, but let's keep it simple for now. |
I will open a PR tomorrow if no one has opened it yet, if someone has, I will contribute tomorrow 🎉. If you guys want, you can assign this issue to me, and I will open a PR tomorrow. Thanks again for this opportunity 🥰 |
@tigerabrodi the kcd-scripts, GitHub Actions and semantic release changes are in master now, in case you hadn't noticed yet. Edit: I didn't notice this had already been mentioned in a previous comment... Carry on. |
I am so excited 😃, I am sort of new to OSS, I think it will take some time for me to relax from all these energetic feelings, I am just way too happy haha 😄 |
@mpeyper Is it a must that the PR should be opened tomorrow 😬, I'd love to work on this with a couple of friends 🥰, but we are currently finishing another PR. Is it okay to open it sometime next week 🤔? Maybe next weekend 🙄? Edit: I will open the PR tomorrow, sorry for all the confusion lol 😄 |
@marcosvega91 Hold on 🙈 |
try to restart |
@marcosvega91 I tried running |
This seems to be wrong 🤔?
|
Yes this error is because you don't have file typescript yet. When you start renaming js files to ts you should not have the problem anymore |
We will try creating a PR this week with a small change from https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/testing-library__react-hooks 👏 🤞 🥰 |
I can convert this library to TS, or is someone already doing this? |
@juhanakristian Should open a PR today 👍 cc @Goldziher |
@Goldziher I think they're planning on doing the conversion fairly openly and collaboratively in #515. Perhaps you can jump on board and help them out? @tigerabrodi? |
@Goldziher You are more than welcome 🥰 🤗 🙌 @juhanakristian Has to invite you to his fork, just ping him confirming that you wanna jump on board 🎉 @Goldziher Not a Requirement 😉 Would be happy if you could join https://kentcdodds.com/discord/, we communicate there in a specific channel, I mean, if you wanna jump on board and help us out, it'd mean a lot 😃 🥳 🥰 |
Will do 😃. Thanks for the invite. |
@tigerabrodi , I joined the server. What channel Am i looking for (this is a huge server). |
Hello, Typescripters! type RendererArray = Array<{ required: string; renderer: string }>
const RENDERERS: RendererArray = [{ required: 'react-test-renderer', renderer: './native/pure' }]
function getRenderer (renderers: RendererArray): string {
const hasDependency = (name: string) => {
try {
require(name)
return true
} catch {
return false
}
}
const [validRenderer] = renderers.filter(({ required }) => hasDependency(required))
if (validRenderer) {
return validRenderer.renderer
} else {
const options = renderers.map(({ renderer }) => ` - ${renderer}`).join('\n')
throw new Error(`Could not auto-detect a React renderer. Options are:\n${options}`)
}
}
export default require(getRenderer(RENDERERS)) and for total transparency here's the code for import { renderHook, act, cleanup } from './pure'
cleanup.autoRegister()
export { renderHook, act, cleanup } |
@joshuaellis I think you are correct that the const { renderHook, act, cleanup } = require(getRenderer(RENDERERS)) as Renderer // this type does not exists AFAIK
export { renderHook, act, cleanup } |
Cheers @mpeyper i'll look into this a bit more with that direciton, I did think that would be the way to go. I've noticed we have some |
It's possible that after your changes |
This was released in v4.0.0 |
Describe the feature you'd like:
Title says it all. I feel like TypeScript has reached a level of adoption where we shouldn't be scared of losing contributors by using it instead of JavaScript.
I believe other
@testing-library
packages are considering or currently being converted, if they aren't already TS.Suggested implementation:
We currently have type definitions in
DefinitelyTyped
which can be used as the basis of the types.I think we can aim to have a straight cut over to a new codebase rather than a progressive migration as there are not that many files to consider.
I think we should look at using TSDX for the tooling unless anyone has other ideas or suggestions in this space.
Describe alternatives you've considered:
N/A
Teachability, Documentation, Adoption, Migration Strategy:
This package should also become the source of truth for the type definitions and the
DefinitelyTyped
version should be deprecated. I'm not sure what the process if to do that if there is one.Other than that I don't think there are any particular considerations to be made for this.
The text was updated successfully, but these errors were encountered: