Make it so the TypeScript definitions work automatically without config

[kentcdodds]

If we follow the path we took in React Testing Library then that'll mean that people wont have to do anything fancy to make the typings/autocomplete show up which would be rad.

Specifically:

1. Add testing-library__jest-dom to DefinitelyTyped
2. Add @types/testing-library__jest-dom to @testing-library/jest-dom dependencies

I just tested it locally and that appears to work. If I have it in my node_modules then it just works™️

Anyone up for this?

[gnapse]

Wouldn't this make it possible that the type definitions lag behind the latest features in the library? I understand that part of the motivation in doing this in RTL and DTL was this:

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.

I'm no expert in TS, but I feel qualified enough to maintain the small needs of jest-dom (which are simpler than the TS needs of RTL/DTL). It's mainly about making sure that PRs that introduce new features or change the external interface make the appropriate updates.

I'll also check if #45 is indeed still an issue. If that's so, then I'd understand the need for this more. Will post my findings here.

[gnapse]

My main gripe with DefinitelyTyped is that the type definitions are usually lagging behind and users are unable to use new features until the type definitions get up to speed with the latest developments in the repo.

Also, last time I checked DefinitelyTyped was this huge repo with all the type definitions of everything under the sun, all in the same place. It's hard to search for existing issues of specific libraries, since it's all under the same list of issues. I appreciate its existence because most libs don't provide typings, understandably, but I always thought (and my experience as a consumer of libraries has confirmed) that if the library itself provides the type definitions (e.g. Formik) the experience is so much better. I've been off using TS actively for a few months now, so not sure if any of the reasons for these perceptions have changed. I'd love to know others' input on all this, and if any of this was taken into consideration when making the decision to go in this direction in RTL/DTL.

[jgoz]

DefinitelyTyped does have a slight advantage when it comes to global typings, since they will be implicitly included for normal TS projects (those that don’t specify a types tsconfig option).

But as a code/typings contributor to a number of projects, I have to agree with @gnapse about the downsides of projects with typings on DT. With the exception of very popular projects like React, the type definitions are often slightly out of date with the actual library and keeping them in sync involves more overhead than projects with typings in the repo (and more still than projects written in TypeScript).

That said, the DT review/merge process is highly automated and their notification bots are very good, so it’s quite streamlined if you are willing to put in the effort of cloning a second (giant) repo and following the instructions.

If the main concern was ease of use, a theoretical option would be to keep the type definitions inside this repo but also have a DT entry that simply does import “@testing-library/jest-dom/extend-expect”; in the global scope. I’m not sure if that’s a common/recommended practice on DT, but it would make the setup instructions for this repo consistent with other projects and keep the actual typings co-located with the code.

[kentcdodds]

Feel free to continue to maintain them yourself if you feel so inclined. I would love it if the types could somehow get added automatically though.

[gnapse]

I just did the following:

npx create-react-app ts-jest-dom-example --typescript
cd ts-jest-dom-example
yarn add --dev @testing-library/react @testing-library/jest-dom

then I changed the src/App.test.tsx file to have this content instead of the default generated by CRA:

import React from 'react';
import { render } from "@testing-library/react";
import "@testing-library/jest-dom/extend-expect";
import App from './App';

it('renders the expected content', () => {
  const { container } = render(<App />);
  expect(container).toHaveTextContent("Learn React");
});

Ran the tests with yarn test and it works. I added the assertion first and vscode's TS warnings immediately complained about toHaveTextContent not being recognized. But as soon as I typed the import line it removed the alert underline.

Granted, I could not make it work by centrally importing jest-dom in a single location (e.g. src/setupTests.ts) instead of having to import it in every single test file that uses it. Maybe that's what you'd like? Because if the above is enough, I'm not sure I'm getting what's the issue.

@jgoz would you happen to know if we can make the centralized import work? I'll try and dig into this tomorrow but now I need some sleep 😅

[kentcdodds]

I could not make it work by centrally importing jest-dom in a single location (e.g. src/setupTests.ts) instead of having to import it in every single test file that uses it. Maybe that's what you'd like?

Precisely. And having it installed via @types/testing-library__jest-dom will make that work. So maybe all we'd need is:

1. A contrib to DefinitelyTyped which simply does import '@testing-library/jest-dom/extend-expect'
2. Add that as a dependency to this project so it gets auto-installed for folks.

I believe that should do it.

[gnapse]

Nice! If that works that'd fix #45, your concerns, and my concerns about divergence of types vs lib.

Although I'll dig a bit more. jest-enzyme do not seem to be on DefinitelyTyped, they have the types in the same repo as we do, and their instructions to make the matchers work in TS are basically what we want to support (import once in setupTests.ts). I'll check if that indeed works in a local repo, and if it does, I'll dig into what's the difference.

[kentcdodds]

I believe that theirs works because the setupTests.ts is a TypeScript file. But if it's a .js file it will not work properly. I could be wrong, but I feel like I read a comment in this repo that said that. Yep, this one.

[gnapse]

Was your example of "it just works when I tried locally" working with the setupTest.js file being JS, importing jest-dom only in this setup file, and then using the custom matchers in TS test files?

Also, an update: it works for me having the centrally import in setupTest.ts. It's just vscode that does not recognizes it 🤔

Also, I do not think it's unreasonable to require, if you're already using TS, that your jest setupTest file be a TS file.

[kentcdodds]

All of my code is regular JS. No TS. And my local test was working great with that when I created a node_modules/@types/testing-library__jest-dom.d.ts which simply did: import '@testing-library/jest-dom/extend-expect'

I do not think it's unreasonable to require, if you're already using TS, that your jest setupTest file be a TS file.

I agree with you. I'm talking about the experience for people not using TypeScript, but using an editor which supports TypeScript autocomplete (like VSCode).

[weyert]

Do you want to make extract it? Shall I make a pull request?

[jgoz]

would you happen to know if we can make the centralized import work?

AFAIK, there’s no way to expose a global type definition directly from an npm module, but it should work with the DT method described above. I won’t be able to test myself for a couple of days though.

[gnapse]

Ok, I agree then to go with what Kent described above.

Regarding this concern from @jgoz

I’m not sure if that’s a common/recommended practice on DT

Indeed, their instructions for creating a new package pretty much assume you're going to add something of substance, not merely a redirector back to your own package. Let's hope that's ok anyway.

[weyert]

I have added a draft pull request for now: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/37688

[weyert]

I think the PR at DefinetlyTyped is good to go

[weyert]

-Merged. Guess, we can now update this package to refer the type definitions :)- Looks like MSFT has closed the pull request, what shall we do?

[johnnyreilly]

I've reopened it and started a discussion. Let's see where it goes.

Just to summarize my understanding: you'd like to remove friction for people acquiring type definitions. @sheetalkamat closed the Definitely Typed PR as this package already has type definitions locally which could be exposed like so:

This change is not needed. The package can add "types" field to the package and thats the desired way of adding types. Refer to https://github.com/Microsoft/TypeScript-Handbook/blob/master/pages/declaration%20files/Publishing.md

This is an entirely valid solution.

The alternative is doing what the React Testing Library did. i.e. dropping local definitions in favour of them being managed in DefinitelyTyped. That's what the linked PR to Definitely Typed by @weyert is about. If that was merged then a PR would follow for @testing-library/jest-dom which dropped the local type definitions and added a dependency for the generated @types/testing-library__jest-dom package.

More details in the suggestion from @kentcdodds here: https://github.com/testing-library/jest-dom/issues/123#issuecomment-521863263

To be entirely clear, my understanding is that the desire is to move to the latter solution (using DT) rather than the former. Is that correct? Or is exposing "types" via package.json being actively considered as well?

[weyert]

Yes, that's the idea to do it for all the @testing-library brother and sister packages. I thought it would be better to sort out the type definitions in TD first before making a PR here

[gnapse]

I thought we were going to first attempt what @kentcdodds described above:

Precisely. And having it installed via @types/testing-library__jest-dom will make that work. So maybe all we'd need is:

1. A contrib to DefinitelyTyped which simply does import '@testing-library/jest-dom/extend-expect'
2. Add that as a dependency to this project so it gets auto-installed for folks. I believe that should do it.

Now, I realize this may not be the kind of type definition packages expected and accepted in DT. If that's the case, and the only option is to entirely move the type definitions to DT and remove them from here, I already expressed my concerns about that above.

However, if completely moving the types to DT removes the friction in using types even for non-TS users, then so be it. But I'm a bit disappointed that no other solution exists. Does it not?

@johnnyreilly would the solution to keep types in this package, and adding a "types" entry to package.json solve @kentcdodds' concerns expressed here

I'm talking about the experience for people not using TypeScript, but using an editor which supports TypeScript autocomplete (like VSCode).

[jgoz]

The fundamental reason these types need to be published on DT in some form is because they are global types that extend Jest's global types.

I.e., most people are probably not doing import {toHaveTextContent} from "@testing-library/jest-dom", they are probably just using expect(foo).toHaveTextContent(...) after configuring Jest.

As far as I know, it is not possible to publish global types from an npm package and have them "just work" when someone installs that package. This is because TypeScript's default type root is @types, so it will only ever look in node_modules/@types (and search up the directory hierarchy for the same) for types that will be automatically included.

So global typings like ours must be published to DT if we don't want to force the user would to do some kind of configuration (like they do now). But we also wanted to be able to keep maintenance of those types inside this repository so there is less chance that they go out of sync with the actual functionality, hence the PR that technically breaks the DT rules.

The flipside of having types on DT, whether directly or as a redirect, is that it might be confusing for new users who don't realize they need to add import "@testing-library/jest-dom/extend-expect" to a Jest configuration file. They could add the dependency and then toHaveTextContent magically appears in their editor, but running the tests would fail.

All of this could be avoided if Jest allowed the setupFilesAfterEnv to be written in TypeScript. That way, the setup file could be added to a user's tsconfig.json and we wouldn't need to publish the types to DT, since they would be explicitly imported as a one-time global import. But since that isn't possible, we have this set of tradeoffs to weigh.

[kentcdodds]

To be clear, here's the tradeoff, we either:

A. Require JS folks to install the types to get autocomplete (most wont). B. Make it automatic without config, but risk people getting autocomplete suggestions when jest-dom has not been configured properly.

When talking about risk, it's always good to consider how the likelihood and the impact. I would say the impact is a moderate-to-low impact of confusion only. It's pretty unlikely that this confusion would lead to a broken build/inability to deploy an app for example. Confusion as an impact is not great, so it'd be nice to avoid.

However, the likelihood is very low I would say. When someone installs @testing-library/jest-dom they'll probably be reading the instructions for setting things up and they'll add the configuration at that point.

So I'd say the risk is moderate-to-low impact and low risk. So the cost is low, and the benefit is high, IMO.

[johnnyreilly]

@johnnyreilly would the solution to keep types in this package, and adding a "types" entry to package.json solve @kentcdodds' concerns expressed here

I believe so yes. Either solution works AFAIK; both give the same good developer experience. It's just a choice around which way you'd like to go.

[jgoz]

@johnnyreilly I don't think the "types" entry on its own would allow things to "just work", since they are global types, so DT is really the only option for automatic inclusion.

[johnnyreilly]

I haven't tried it but I have a feeling that either solution will work. I think that this being here:

https://github.com/testing-library/jest-dom/blob/170b5ed9d0593c588615a54be84062a1b44f9731/extend-expect.d.ts#L1

will extend the jest namespace which is already pulled in globally. I'm not 💯% on this; but it's easy to test out. I recommend just adding the "types" entry and seeing if it works. If it doesn't then your decision is made 😄

[jgoz]

I'm not 💯% on this; but it's easy to test out. I recommend just adding the "types" entry and seeing if it works. If it doesn't then your decision is made 😄

I should have mentioned that I tried this out last night and it only worked if you added import "@testing-library/jest-dom/extend-expect" to a TS file that is included for compile.

[kentcdodds]

To be clear, the suggestion is that if we add a "types" entry to the package.json of this project that'll make it work automatically as well? I tried that locally just now and it didn't seem to make any difference. I could have done it incorrectly though.

[johnnyreilly]

ah well then. Decision made!

[johnnyreilly]

To be clear, the suggestion is that if we add a "types" entry to the package.json of this project that'll make it work automatically as well?

Yes. Does the PR that @weyert has raised represent the desired way forward? This suggestion made me think that might not be the case...:

All of my code is regular JS. No TS. And my local test was working great with that when I created a node_modules/@types/testing-library__jest-dom.d.ts which simply did: import '@testing-library/jest-dom/extend-expect'

[jgoz]

@kentcdodds

So I'd say the risk is moderate-to-low impact and low risk. So the cost is low, and the benefit is high, IMO.

Yep, makes sense to me. I just wanted to point out that automatically including the types was a potential source of confusion on the other end.

@johnnyreilly

Yes. Does the PR that @weyert has raised represent the desired way forward? This suggestion made me think that might not be the case...:

The original PR on DT was per the description you quoted. It looks like @weyert has since changed it to include all the types. The original PR would have allowed the definitions to be maintained inside of this repo (and was preferred by @gnapse).

[johnnyreilly]

Okay - well should the PR on definitely typed go in as is? Or should it be as @gnapse originally had it?

[jgoz]

It would be nice to revert it and maybe flesh out the explanation for why we are breaking/bending the DT rules. There may be some timezone differences at play here, so I'm not sure how quickly @weyert can update the PR.

[johnnyreilly]

Cool - well let me know when you're ready to go and I'll review

[johnnyreilly]

Side question @jgoz - @basarat recently mentioned to me that back in the day you were one of the first maintainers of a TypeScript loader for webpack. And that his work with Pete Hunt, yourself and someone called "Andrey" all formed the prior art that @jbrantly eventually built upon to make ts-loader which I've been running for quite some time. Are you he? 😄

[weyert]

Sorry, I am confused. What the final decision we have made now? I am happy to update the PR when it's clear what's expected of me and the PR :)

[kentcdodds]

I think the decision is my original suggestion:

1. Add testing-library__jest-dom to DefinitelyTyped
2. Add @types/testing-library__jest-dom to @testing-library/jest-dom dependencies

[kentcdodds]

Ah! Sorry, lol, that's NOT the decision. The decision was my second suggestion:

1. A contrib to DefinitelyTyped which simply does import '@testing-library/jest-dom/extend-expect'
2. Add that as a dependency to this project so it gets auto-installed for folks.

[weyert]

Sorry, let me double check, have been fighting with MobX+RTL too much today at work :)

You want to change the setup for this package from the other @testing-library packages? Leave the type definition in this package, and add a dependency to @testing-library/jest-dom in the type definition ?

[kentcdodds]

Yes, the maintainers here do want to keep the typings in this project, but we still want to make the typings automatic, which is why we're making this change. So the type defs stay here, but we want to add a type def to DT which basically just imports the type defs from this package, and then add the DT def module as a dependency in @testing-library/jest-dom. So there will be a bit of a circular dependency situation, but it should work without any trouble.

[jgoz]

@johnnyreilly

Are you he? 😄

He is me 😄

[weyert]

Sounds odd to me but okay 😀 Let's ask what the DT people think of this plan before making the change

[johnnyreilly]

He is me 😄

Amazing! Small world. ts-loader says "hi!" https://github.com/TypeStrong/ts-loader 😄

[sheetalkamat]

@kentcdodds We wont accept the DT PR if the types continue to leave here.. We recommend that types are present either in package or on DT (definitely not in both places) From https://github.com/Microsoft/TypeScript-Handbook/blob/master/pages/declaration%20files/Publishing.md

If your package is written in TypeScript then the first approach is favored. Use the --declaration flag to generate declaration files. This way, your declarations and JavaScript will always be in sync. If your package is not written in TypeScript then the second is the preferred approach.

[weyert]

Oh just asked about this (https://github.com/DefinitelyTyped/DefinitelyTyped/pull/37688#issuecomment-523193247)

[jgoz]

@sheetalkamat Having an @types package is the only way to include global type definitions simply by adding a dependency (without also adding some kind of configuration). We were hoping to be able to "have our cake and eat it" by co-locating maintenance of types and source code. Are there no exceptions for cases like these?

[kentcdodds]

Considering that, and the fact that @gnapse would prefer to keep types in this package, then it's technically impossible to achieve this automatic type installation and therefore this issue should be closed.

[weyert]

Okay, closed the PR :)

[jgoz]

@kentcdodds Let's wait to hear from @gnapse before closing this issue. If the DT hack is not an option, I would vote for moving the typings to DT.

[gnapse]

I still think that having them in DT is suboptimal, but I too vote the way @jgoz did. It's a pity because I always thought that if libs had the types included it was always the best experience.

Update: For the record, with the type definitions externalized, I will probably not participate actively in any maintenance tasks for the types, aside from intervening in PRs, commenting, etc. It's a huge overhead to have to maintain the types elsewhere. Not to mention clone that huge repo :(

[jgoz]

Yeah, it's unfortunate that this project falls into the narrow pit that doesn't have out-of-box support for included types. Hosting the types on DT shouldn't make a difference from the users' perspective and might improve the overall experience, but it will make maintenance more onerous.

Also, I suppose if someone is using a strict package manager like pnpm that doesn't hoist dependencies, they won't get the "just works" experience, either... but we're well within the 80/20 rule on that I'm sure.

[jgoz]

https://github.com/DefinitelyTyped/DefinitelyTyped/pull/37792