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

fix(typescript): move typings to DefinitelyTyped #437

Merged
merged 2 commits into from
Aug 15, 2019

Conversation

alexkrolick
Copy link
Collaborator

@alexkrolick alexkrolick commented Aug 11, 2019

See #436, testing-library/dom-testing-library#337

Remove the TypeScript typedefs from this repo in favor of having the community maintain them at DefinitelyTyped.

Why

There are no core teammembers with the TS knowledge needed to maintain the types, and they are not well tested or integrated with the codebase.


See also reduxjs/redux#3500

nickserv
nickserv previously approved these changes Aug 12, 2019
@nickserv
Copy link
Member

nickserv commented Aug 12, 2019

It would be best to wait to merge until the types are published in DefinitelyTyped and then increment the major version number.

@eps1lon
Copy link
Member

eps1lon commented Aug 12, 2019

There are no core teammembers with the TS knowledge needed to maintain the types, and they are not well tested or integrated with the codebase.

This seems a bit too eager I feel. The issue is open for 9 hours, you opened this one 2 hours after the original issue was submitted. If the expectation for members with TS knowledge is to fix them within 2 hours then yes there are none.

Other than that I'm happy to fix #436. It might even be possible so that we don't need to maintain act typings.

@afontcu
Copy link
Member

afontcu commented Aug 12, 2019

As far as I can tell, even Microsoft suggests to use DefinitelyTyped if the project isn't written in TypeScript:

If your package is not written in TypeScript then the second is the preferred approach. (that is, publishing to the @types organization on npm).

(source)

@eps1lon
Copy link
Member

eps1lon commented Aug 12, 2019

I would always recommend this as well. But the way this should be done is by creating definitions in @types first before removing those here i.e. offer a migration path for a breaking change.

@eps1lon eps1lon added the TypeScript Related to TypeScript. Note: only certain maintainers handle TypeScript labeled issues. label Aug 12, 2019
@danielkcz
Copy link
Contributor

danielkcz commented Aug 12, 2019

I want to point that moving to DefinitelyTypes doesn't automatically ensure up-to-date declarations. Unless someone from the community can step up, it will become stale maybe even faster than having it inside the repo. Hopefully, RTL is popular enough for that not to happen.

From a user point of view, it's much easier having types in the package so they don't need to be installed separately.

What about having a index.d.ts in a package that will reexport @types/* and add those to dependencies? It's such a small dependency it will hardly matter (especially for dev tool) and it gives the best of two worlds.

@nickserv
Copy link
Member

I want to point that moving to DefinitelyTypes doesn't automatically ensure up-to-date declarations. Unless someone from the community can step up, it will become stale maybe even faster than having it inside the repo. Hopefully, RTL is popular enough for that not to happen.

No maintenance is automatic, but putting types in the same package means that maintainers are responsible for reviewing TypeScript pull requests that they may not understand, when there's already a large community of TypeScript developers at DefinitelyTyped that help with reviewing pull requests.

From a user point of view, it's much easier having types in the package so they don't need to be installed separately.

Having types in the same package is much worse for users. If there's a breaking change in the types, you either don't update the major version in which case TypeScript users get an unpleasant surprise (happens more frequently when package maintainers don't use TypeScript themselves), or you do in which case the version keeps changing for TS and JS users when it only affects TS users. Version management is much easier when types are in a separate package (except for when the package is written in TS, which keeps the types in sync).

What about having a index.d.ts in a package that will reexport @types/* and add those to dependencies? It's such a small dependency it will hardly matter (especially for dev tool) and it gives the best of two worlds.

Are you suggesting that @types/testing-library__react depends on the other @types/testing-library__* packages? This is a good idea, and is recommended by the TypeScript team.

@weyert
Copy link
Contributor

weyert commented Aug 12, 2019

I am using TypeScript at work and happy to keep them up to date a:)
My colleague is part of DefinetlyTyoed so should be able to get PR reasonable quickly merged :)

@alexkrolick
Copy link
Collaborator Author

What about having a index.d.ts in a package that will reexport @types/* and add those to dependencies? It's such a small dependency it will hardly matter (especially for dev tool) and it gives the best of two worlds.

Are you suggesting that @types/testing-library__react depends on the other @types/testing-library__* packages? This is a good idea, and is recommended by the TypeScript team.

I think @FredyC meant @testing-library/react could have @types/testing-library__react as a dependency and re-export it. I am not sure if that is recommended, it's the first time I'm hearing of that approach.

@weyert would you be willing to start a PR to DT to add the types? As others have pointed out the responsible approach is to get those ready first to ensure a migration path.

@weyert
Copy link
Contributor

weyert commented Aug 12, 2019

Sure, I will have a look st it tomorrow. Earlier I already made one for user-event :)

@danielkcz
Copy link
Contributor

danielkcz commented Aug 12, 2019

@alexkrolick

I think @FredyC meant @testing-library/react could have @types/testing-library__react as a dependency and re-export it.

You got that correctly. I never heard of it either, but it just occurred to me it could solve both pains at once. Of course, it means install extra micro dependency for non-TS users, but that's hardly a problem in this case imo.

@nickmccurdy

Having types in the same package is much worse for users.

If I understand you correctly, you mean this in case the typings are out of date? Sure, that's a problem. I meant it more generally, be it that project is written in TypeScript or not, with up to date typings it's better DX than having to check if @types is available and install it.

@weyert
Copy link
Contributor

weyert commented Aug 12, 2019 via email

@danielkcz
Copy link
Contributor

danielkcz commented Aug 12, 2019

I can imagine that inherited types of dom package in the React type definitions would be possible

Sure, that is one part, but I am talking about having index.d.ts in this package that reexports @types/testing-library__react. That way user doesn't need to install types and they will be maintained over at DT. It won't even be a breaking change from the user point of view.

@afontcu
Copy link
Member

afontcu commented Aug 12, 2019

I see that several Testing Library projects have types-related debates open (RTL, DTL, VTL, user-event...). I wonder if there's a way of avoiding repetition and to provide the same experience to our users regardless of the Testing Library flavor or utility they're using.

Since DTL is the foundation of many packages, I guess RTL and VTL types would extend from DTL's and customize them accordingly. Am I right?

Does it make sense? Is this the direction we are going? 🙌 Maybe the debate should be unified too, so all efforts are focused?

@weyert
Copy link
Contributor

weyert commented Aug 12, 2019

Does it make sense? Is this the direction we are going? 🙌 Maybe the debate should be unified too, so all efforts are focused?

Yeah, that's the idea I am having for now. I think that would be the most convenient way to get the query type defs into the projects?

@nickserv
Copy link
Member

nickserv commented Aug 12, 2019

@FredyC

@nickmccurdy

Having types in the same package is much worse for users.

If I understand you correctly, you mean this in case the typings are out of date? Sure, that's a problem. I meant it more generally, be it that project is written in TypeScript or not, with up to date typings it's better DX than having to check if @types is available and install it.

You're right that the types themselves are going to have problems whether or not they're in the same package, but my point is that version management is difficult to do well without a separate types package. This is one of the reasons why TypeScript recommends removing types from packages not written in TypeScript.

@kentcdodds
Copy link
Member

For the record. I'm in favor of these changes. I think it'll be better for everyone. Thanks for working on it @weyert!

@cimbul
Copy link

cimbul commented Aug 13, 2019

@weyert, are you also planning on handling the DefinitelyTyped PR for dom-testing-library? I've got a DT PR ready for VTL (see discussion on testing-library/vue-testing-library#74), but I'm waiting on DTL to be moved there first. If you were only going to handle RTL, I can take a pass at it, but I wasn't clear.

@weyert
Copy link
Contributor

weyert commented Aug 13, 2019

I am happy to take it over :)

@weyert
Copy link
Contributor

weyert commented Aug 13, 2019 via email

@MichaelDeBoey
Copy link
Member

The redux repo is more leaning towards rewriting the code in TS.

Is this something we want to consider too for the testing-library packages?
Maybe only DTL (since redux is considered, but redux-thunk, reselect, ... aren’t at this moment)? 🤔

@kentcdodds
Copy link
Member

I've held back from writing my OSS in TypeScript because I don't want to raise the barrier for contributors. So no, I don't think that's what we're going to do.

BREAKING: remove TypeScript definitions from repo
@allcontributors
Copy link
Contributor

@kentcdodds

I've put up a pull request to add @FredyC! 🎉

@kentcdodds
Copy link
Member

Phew 😌 I think that's everyone. Thanks a TON for all the help here :) I really think this'll be better for everyone!

@kentcdodds kentcdodds changed the title chore(typescript): remove typings fix(typescript): move typings to DefinitelyTyped Aug 15, 2019
@kentcdodds
Copy link
Member

🎉 This PR is included in version 9.1.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@alexkrolick
Copy link
Collaborator Author

Great work everyone! 🚀

@typeofweb
Copy link

Since this is a breaking change, shouldn't it be released as 10.0.0?

@danielkcz
Copy link
Contributor

danielkcz commented Aug 16, 2019

@mmiszy It's not breaking change because DT types are as a dependency of RTL (and DTL)

@typeofweb
Copy link

@FredyC does it mean that types are kind of reexported and just work? Thanks for clearing this up :)

@johnnyreilly
Copy link

I've written this PR up as a blog post that I'll probably publish over the next couple of days. Unless anyone objects I'm going to quote a few of the comments and screenshots in here. Do let me know if there's any objections. I'll share the link here when I publish the blog. I think there's some useful lessons for the community in here around type maintenance / distribution

@kentcdodds
Copy link
Member

I was thinking about writing it myself. I'm glad you're doing it so I don't have to!

If you could make sure that our motivations are clear that would be great:

We were getting a lot of drive-by contributions to the TypeScript typings and many pull requests would either sit without being reviewed by someone who knows TypeScript well enough, or be merged by a maintainer who just hoped the contributor knew what they were doing. This resulted in a poor experience for TypeScript users who could experience type definition churn and delays, and it became a burden on project maintainers as well (most of us don't know TypeScript very well). Moving the type definitions to DefinitelyTyped puts the maintenance in much more capable hands. And by adding the type definitions to the dependencies of React Testing Library, the experience for users is completely unchanged. So it's a huge improvement for the maintenance of the type definitions without any breaking changes for the users of those definitions.

I look forward to reading your blog post!

@johnnyreilly
Copy link

Awesome - thanks Kent!

Yes - I'll be sure to make clear the motivations behind this. What I've written roughly mirrors what you've said. I'll probably quote you more directly as I think you've summed it up well.

@johnnyreilly
Copy link

Blogged! https://blog.johnnyreilly.com/2019/08/symbiotic-definitely-typed.html

Let me know if there's any corrections / clarifications you'd like. Thanks for your help!

@weyert
Copy link
Contributor

weyert commented Aug 17, 2019

Wow great! Thanks for taking the time to write the blog article :)

@danielkcz
Copy link
Contributor

danielkcz commented Aug 18, 2019

@johnnyreilly Great article. I don't want to nitpick or collect medals, but I believe it was actually my idea (although rough around the edges) to add DT types as dependency ;) Unless Kent has done that somewhere else earlier.

@kentcdodds
Copy link
Member

Almost. Your idea was to re-export them. My idea was to just add them as a dependency so they're installed, but not bother with a re-export. But we're splitting hairs. I do think everyone should be credited for the discussion here.

@danielkcz
Copy link
Contributor

@kentcdodds Yea, I just wasn't sure it will work by only installing that dep, so I went with a path I know :) You have polished it.

@johnnyreilly
Copy link

johnnyreilly commented Aug 18, 2019

I'll update the article to reflect @FredyC

Done - I also added in a point about ensuring a loose version range suggested by @andrewbranch.

Copy link

@oudomskn oudomskn left a comment

Choose a reason for hiding this comment

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

Bill

@FreightCompanionDavid
Copy link

This was such a good read I know I've got full blown nerd ><

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released TypeScript Related to TypeScript. Note: only certain maintainers handle TypeScript labeled issues.
Projects
None yet
Development

Successfully merging this pull request may close these issues.