Skip to content

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

Closed
mpeyper opened this issue Dec 4, 2020 · 52 comments
Closed

Convert to TypeScript #498

mpeyper opened this issue Dec 4, 2020 · 52 comments
Labels
enhancement New feature or request released

Comments

@mpeyper
Copy link
Member

mpeyper commented Dec 4, 2020

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.

@mpeyper mpeyper added enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers labels Dec 4, 2020
@joshuaellis
Copy link
Member

The compiler won't recommend looking for @types/* library anymore because we'll have bundled types. So I imagine they would be able to put a depreciated flag on the package?

Is there a reason you think we should tsdx? Looking at the repo, it's pretty immature and there's a few outstanding issues. I think we could probably just use our own configuration. Proabably easier to maintain overall.

@nickserv
Copy link
Member

nickserv commented Dec 4, 2020

I think we should look at using TSDX for the tooling unless anyone has other ideas or suggestions in this space.

kcd-scripts now supports TypeScript and it's already the de facto standard tool for this organization.

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.

There's a list of removed packages that you can submit a pull request to.

@mpeyper
Copy link
Member Author

mpeyper commented Dec 4, 2020

Is there a reason you think we should tsdx?

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.

@marcosvega91
Copy link
Member

If you want I can migrate the actual codebase to kcd-scripts.

@nickserv
Copy link
Member

nickserv commented Dec 4, 2020

In that case it's probably easier to do #499 with yo kcd-oss first. We'll have better CI for this PR and TS tooling setup will be simpler.

@marcosvega91
Copy link
Member

Yes, sure

@mpeyper mpeyper mentioned this issue Dec 4, 2020
4 tasks
@tigerabrodi
Copy link
Contributor

@marcosvega91 @mpeyper I'd love to help when it comes to migrating to TypeScript 💕

@mpeyper
Copy link
Member Author

mpeyper commented Dec 5, 2020

@tigerabrodi

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)

@tigerabrodi
Copy link
Contributor

tigerabrodi commented Dec 5, 2020

@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 🙏 💕

@mpeyper
Copy link
Member Author

mpeyper commented Dec 5, 2020

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).

@tigerabrodi
Copy link
Contributor

@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! 🙏

@mpeyper
Copy link
Member Author

mpeyper commented Dec 5, 2020

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.

@nickserv
Copy link
Member

nickserv commented Dec 6, 2020

You can also use the Open with button if you have GitHub Desktop or Codespaces, otherwise you can install gh and use gh pr checkout 502.

@tigerabrodi
Copy link
Contributor

Will do it later, two tasks to cross off my side project, I am looking forward to it though haha 😍 💯

Probably just install gh then do what @nickmccurdy said 🎉

Thank you guys, you are so awesome 😄

@nickserv
Copy link
Member

nickserv commented Dec 6, 2020

#502 has been merged, so at this point you can pull down master and branch off it directly.

@tigerabrodi
Copy link
Contributor

Yeah lit 🔥 🤘, looking forward to it 😄

@tigerabrodi
Copy link
Contributor

@nickmccurdy @mpeyper @marcosvega91 I could create the first PR with a small change, or how do you want to have it?

@nickserv
Copy link
Member

nickserv commented Dec 6, 2020

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.

@tigerabrodi
Copy link
Contributor

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?

@nickserv
Copy link
Member

nickserv commented Dec 6, 2020

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.

@tigerabrodi
Copy link
Contributor

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 🥰

@mpeyper
Copy link
Member Author

mpeyper commented Dec 6, 2020

@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.

@tigerabrodi
Copy link
Contributor

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 😄

@tigerabrodi
Copy link
Contributor

tigerabrodi commented Dec 6, 2020

@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 😄

@tigerabrodi
Copy link
Contributor

@marcosvega91 🥰

@tigerabrodi
Copy link
Contributor

@marcosvega91 Hold on 🙈
Screenshot from 2020-12-07 19-11-17

@marcosvega91
Copy link
Member

marcosvega91 commented Dec 7, 2020

try to restart typescript server. It should be CTRL + SHIFT + P and then type typescript server

@tigerabrodi
Copy link
Contributor

tigerabrodi commented Dec 7, 2020

Seems like it does take some time to initialize 🙈
Screenshot from 2020-12-07 19-25-02

@tigerabrodi
Copy link
Contributor

@marcosvega91 I tried running npm i to see what would happen because it is not finishing initialization.

I get this error:
Screenshot from 2020-12-07 19-34-27

@tigerabrodi
Copy link
Contributor

This seems to be wrong 🤔?

error TS18003: No inputs were found in config file '/home/tigerabrodi/Desktop/open-source/react-hooks-testing-library/tsconfig.json'. Specified 'include' paths were '["node_modules/kcd-scripts/../../src/**/*"]' and 'exclude' paths were '["node_modules/kcd-scripts/node_modules"]'

@marcosvega91
Copy link
Member

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

@tigerabrodi
Copy link
Contributor

tigerabrodi commented Dec 9, 2020

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 👏 🤞 🥰

@Goldziher
Copy link
Contributor

I can convert this library to TS, or is someone already doing this?

@tigerabrodi
Copy link
Contributor

tigerabrodi commented Dec 9, 2020

@juhanakristian Should open a PR today 👍

cc @Goldziher

@mpeyper
Copy link
Member Author

mpeyper commented Dec 9, 2020

@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?

@mpeyper mpeyper assigned tigerabrodi and unassigned tigerabrodi Dec 9, 2020
@tigerabrodi
Copy link
Contributor

@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 😃 🥳 🥰

@Goldziher
Copy link
Contributor

Will do 😃. Thanks for the invite.

@Goldziher
Copy link
Contributor

@tigerabrodi , I joined the server. What channel Am i looking for (this is a huge server).

@mpeyper mpeyper added released on @beta and removed good first issue Good for newcomers help wanted Extra attention is needed labels Dec 15, 2020
@joshuaellis
Copy link
Member

Hello, Typescripters!
I'm working on #68 and one of the features we wanted to include was a form of auto-selecting the correct test renderer based on the user's devDependencies. I'm having a bit of difficulty writing this in Typescript – when you import ./pure into index you get the following issues:
image
So, I'm wondering if anyone had a suggestion? I already can guess it'll be to do with the require call. Here's the code for pure.

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 index.ts

import { renderHook, act, cleanup } from './pure'

cleanup.autoRegister()

export { renderHook, act, cleanup }

@mpeyper
Copy link
Member Author

mpeyper commented Dec 23, 2020

@joshuaellis I think you are correct that the require call is unable to determine the type. I think we'll end up needing something like:

const { renderHook, act, cleanup } = require(getRenderer(RENDERERS)) as Renderer // this type does not exists AFAIK
export { renderHook, act, cleanup }

@joshuaellis
Copy link
Member

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 @types in depencies, just wondering if they should be there or devDependencies. I'm not sure personally but I wanted to raise it, I can also ensure when I add react-dom I put that @types into the correct place.
image

@mpeyper
Copy link
Member Author

mpeyper commented Dec 30, 2020

dependencies is correct for these ones as our public API uses their types so our TS consumers need them too.

It's possible that after your changes react-dom and react-test-renderer would become devDependencies as we won't be able to rely on either's types in our public API.

@joshuaellis
Copy link
Member

This was released in v4.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

No branches or pull requests

6 participants