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

Convert codebase to TypeScript #494

Open
abraham opened this issue Mar 22, 2020 · 20 comments
Open

Convert codebase to TypeScript #494

abraham opened this issue Mar 22, 2020 · 20 comments
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@abraham
Copy link

abraham commented Mar 22, 2020

Describe the feature you'd like:

Accurate and timely TypeScript types.

Currently types are manually maintained in the @types/ project so they are only updated after the fact and are prone to human error and implementation drift.

Suggested implementation:

Convert @testing-library/dom to TypeScript.

Describe alternatives you've considered:

The main alternative is to leave things the way they are. Unfortunately this leaves types manually updated after the fact. This is slow, depend on third-parties (Microsoft), and is error prone.

I'm sure there are other issues filed on the DefinitelyTyped repo.

Another alternative is to require DefinitelyTyped types as part of general contributions to this project. This would be a big increase to the barrier of entry as contributors would have to know how to write types and update DefinitelyTyped.

Teachability, Documentation, Adoption, Migration Strategy:

I'd look at doing a multi-stage conversion

  1. Initial proof of concept with one or two files to validate the approach and tooling.
  2. The src files
  3. Updated contributing guide with guidance on types
  4. The tests files (this could be delayed until a future date)
  5. Update testing-library.com documentation to display the latest types (optional but if they exists they might as well be used)

TypeScript does add some additional complexity in tooling and would increase the barrier of entry for contributors. This is also an opportunity to to create a simple and supportive implementation that people less familiar with TypeScript and contribute to. It would be important to create some good contributing guides on working with types (or identify and recommend some good third-party guides).

Potentially there would be bugs uncovered and fixed in the codebase with the conversion.

@kentcdodds
Copy link
Member

I definitely understand the desire here. I'll hold on giving my opinion until I hear what other maintainers have to say about it.

@eps1lon
Copy link
Member

eps1lon commented Mar 23, 2020

Couple of loose points that are not meant as a final 👍 or 👎

  1. Types lagging behind is an issue but so are patch and feature releases that break typings. People get very frustrated with these from my experience which is why separate packages for runtime and types are the perfect solution: people valuing soundness can upgrade their types immediately while people valuing non-breaking changes can wait with their type updates
  2. One of the original reasons to outsource them was that we didn't have anyone with TypeScript experience. This didn't apply back then (unless you discount me) and it shouldn't apply right now.
  3. TypeScript might be intimidating for contributors. Either they don't start the effort or stop because they think they shouldn't submit a PR with errors. However, I'm not aware of any case studies so "typescript -> less contributions" might not hold true in the end.
  4. We could help 3 by simply publishing the types from this repository again. We have plenty of steps in between "not oudated typs" and "convert to typescript". The migration path is one of the great strengths of TypeScript. We don't have to switch our tooling. We can start by separate declarations, @ts-check, source files in .ts (with babel transpiling).

On a personal note: The @types/ namespace should not be owned by DefinitelyTyped or microsoft. I don't find it fair that they have the monopoly on what "quality types" are. Especially considering this oftentimes means: outdated types. It should be possible for projects to publish their types to @types/

PS:
Regarding "what do I do if I don't know typescript":
I see this misconception a lot from people that are newer to open source. At least in the projects I maintain you don't have to write the perfect PR. This is a collaborative effort and therefore it's ok for you to write the first iteration and another one adds types.
For people struggling with typescript I would suggest using "type coverage" as a metric. You don't have to make the perfect types on your first iteration. It's good enough to start with string or number or {}. These can be refined later with interfaces and generics and unions and intersections and whatever fancy group theory tool typescript has. Whenever typescript throws a cryptic error at you: any it or @ts-ignore it and ping me. I'll gladly help.

@eps1lon
Copy link
Member

eps1lon commented Mar 23, 2020

On the subject of TypeScript holding of potential contributions: We also use contributor-hostile pre-commit hooks. So if there's any concern that beginners might not contribute I would start by removing pre-commit hooks.

@davisford
Copy link

davisford commented Mar 23, 2020

I'm wrestling now with a type / mis-match / version mess using testing-library/react and testing-library/dom. I upgraded -- both had major version number upgrades. I read the breaking changes. The only thing I needed to address was the switch from wait to waitFor. Unfortunately, there were dozens of other typing issues, so I've backed it out now to the following:

    "@testing-library/dom": "^6.16.0",
    "@testing-library/jest-dom": "^5.1.1",
    "@testing-library/react": "^9.5.0",

This pulls in transitive types:

│ ├─┬ @types/testing-library__dom@6.14.0
│ ├─┬ @types/testing-library__jest-dom@5.0.2
│ └─┬ @types/testing-library__react@9.1.3

None of these are working for me. I have tons of breaking import problems when I try to build or run my tests. Here's an example from @types/testing-library__react@9.1.3 -- it can't even resolve the exported types from @types/testing-library__dom@6.14.0.

Screen Shot 2020-03-23 at 10 57 09 AM

This is mega-frustrating. Can anyone advise on a release version that works? The last combo I had that worked for us was:

    "@testing-library/dom": "^6.8.0",
    "@testing-library/jest-dom": "^4.2.4",
    "@testing-library/react": "^9.3.0",

After trying to do minor semver upgrade or major, nothing works again. I would love it if either this codebase were converted entirely to TS, or you publish the types with the code again so they track with each release. I can never tell which definitely typed version is supposed to match which release. There's very little effort to document that, other than the transitive dependency which is pulled, but as stated earlier, that is fraught with problems now.

Also note related discussion in this react-testing-library issue

@kentcdodds
Copy link
Member

I'm willing to move the typings back into our repo as separate .d.ts files to start. Once we have that working than we can see about rewriting to TypeScript.

Who wants to work on the first phase?

@eps1lon
Copy link
Member

eps1lon commented Mar 23, 2020

I'm willing to move the typings back into our repo as separate .d.ts files to start.

the crowd cheers

Who wants to work on the first phase?

the crowd grows silent

@kentcdodds
Copy link
Member

I want to add really quick here for anyone who might be working on this: For the first PR, PLEASE keep it as small as possible or at least make it easy to follow what's going on commit-by-commit. I don't want to review a monster PR that changes everything in the codebase. Thanks <3

@nickserv

This comment has been minimized.

@kentcdodds

This comment has been minimized.

@nickserv

This comment has been minimized.

@kentcdodds

This comment has been minimized.

@nickserv nickserv added the help wanted Extra attention is needed label Oct 6, 2020
@nickserv
Copy link
Member

I'm thinking of starting with the waiting functions since they're commonly reused, and ending with the query functions since why seem complex and will probably involve higher level mapped types.

@riotrah
Copy link
Contributor

riotrah commented Aug 5, 2021

Hi guys, apologies for adding noise to this issue if it's been abandoned, completed, or otherwise resolved. TL;DR: How can I help? What's the status of this issue?

I noticed that there have been somewhat recently merged contributions pertaining to TS, but not any formal announcements or progress tracked in this issue - so may I ask what the progress or state of this is? I wanted to ask because some comments above pertaining to suggested scoping for first contributions by @nickmccurdy have been hidden, so I didn't know if there are currently any preferences by the maintainers as to how someone can contribute.

Further adding to my (perhaps unwarranted) confusion is that there appear to be some MRs that are explicit TS conversions vs inlined or imported definitely-typed declarations. So what is the road we're taking?

@nickserv
Copy link
Member

nickserv commented Aug 6, 2021

I'll try to track the related PRs here, then we can split up work on any remaining files:

I wanted to ask because some comments above pertaining to suggested scoping for first contributions by @nickmccurdy have been hidden

I think these were hidden because they were already resolved, but you should still be able to expand them with Show comment.

Further adding to my (perhaps unwarranted) confusion is that there appear to be some MRs that are explicit TS conversions vs inlined or imported definitely-typed declarations. So what is the road we're taking?

I think we previously agreed to gradually move all our packages to first class TypeScript conversion. We may still leave some declarations around until the remaining JS files have been converted though.

@riotrah
Copy link
Contributor

riotrah commented Aug 6, 2021

As of Aug 6th the 4 PRs references in the comment immediately before mine convert (or attempt to) the following files:

  1. feat: migrate suggestions/pretty-dom/helpers/role-helpers to ts #850
  • src/suggestions.js
  • src/pretty-dom.js
  • src/helpers.js
  • src/role-helpers.js
  1. Move to TS #954
  • src/event-map.js
  • src/get-queries-for-element.js
  • src/get-user-code-frame.js
  • src/index.js
  • src/queries/role.js
  1. Draft: added typescript types to waitfor functions #982
  • src/helpers.js
  • src/wait-for-dom-change.js
  • src/wait-for-element-to-be-removed.js
  • src/wait-for-element.js
  • src/wait-for.js
  1. Draft: convert file get-user-code-frame.js to TypeScript #983
  • src/get-user-code-frame.js

@riotrah
Copy link
Contributor

riotrah commented Aug 6, 2021

Here's what I'm tackling. I'll update before and after every PR.

Edit: My PRs/Files changed:

  1. chore: Convert screen.js to TypeScript #1002
  • src/screen.js
  1. TS Conversion > src/events.js + edit src/events.d.ts #1008
  • src/events.js
  1. TS Conversion > src/query-helpers.js #1009
  • src/query-helpers.js

riotrah pushed a commit to riotrah/dom-testing-library that referenced this issue Aug 6, 2021
riotrah pushed a commit to riotrah/dom-testing-library that referenced this issue Aug 6, 2021
riotrah pushed a commit to riotrah/dom-testing-library that referenced this issue Aug 12, 2021
@riotrah
Copy link
Contributor

riotrah commented Aug 18, 2021

With my third open PR above (#1009 ), plus my others and the rest I've listed 2 comments prior, the work of converting all the remaining files in src/ to TS are completely contained within currently open PRs.

My question to maintainers:

  1. Barring work stemming from feedback to my own open PRs, is there anything I, or the community, can do further?
  2. Some PRs have had feedback provided without action taken by authors, is this something we can help with?
  3. Are we interested in migrating the tests to TS? Or is there anything work in the realm of types that is left?

@kentcdodds
Copy link
Member

I'm sorry, I'm afraid I don't have the bandwidth to work on this 😬

@mnindrazaka
Copy link

Hi guys, I'm a first-time contributor and want to help with this issue. Is there anything left that needs to be migrated to typescript? If not, is there any other issue that might be good for me ?

@RotemCarmon
Copy link

Hey, is there still anything that needs to be done on this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

9 participants