Make it so the TypeScript definitions work automatically without config #123
Comments
|
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:
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. |
|
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. |
|
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 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 |
|
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. |
kentcdodds
changed the title
|
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-domthen I changed the 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 Granted, I could not make it work by centrally importing @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 😅 |
Precisely. And having it installed via
I believe that should do it. |
|
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. |
|
I believe that theirs works because the |
|
Was your example of "it just works when I tried locally" working with the Also, an update: it works for me having the centrally import in Also, I do not think it's unreasonable to require, if you're already using TS, that your jest |
|
All of my code is regular JS. No TS. And my local test was working great with that when I created a
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). |
|
Do you want to make extract it? Shall I make a pull request? |
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. |
|
Ok, I agree then to go with what Kent described above. Regarding this concern from @jgoz
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. |
|
I have added a draft pull request for now: DefinitelyTyped/DefinitelyTyped#37688 |
|
I think the PR at DefinetlyTyped is good to go |
|
-Merged. Guess, we can now update this package to refer the type definitions :)- |
|
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 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 More details in the suggestion from @kentcdodds here: #123 (comment) 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 |
|
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 |
|
I thought we were going to first attempt what @kentcdodds described above:
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
|
|
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 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 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 All of this could be avoided if Jest allowed the |
|
To be clear, here's the tradeoff, we either: A. Require JS folks to install the types to get autocomplete (most wont). 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 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. |
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. |
|
@johnnyreilly I don't think the |
|
I haven't tried it but I have a feeling that either solution will work. I think that this being here: Line 1 in 170b5ed will extend the |
I should have mentioned that I tried this out last night and it only worked if you added |
|
To be clear, the suggestion is that if we add a |
|
ah well then. Decision made! |
|
Hi, |
|
For To resolve, I just added |
|
An alternative approach for |
|
I think adding to tsconfig |
pnpm installs these types to node_modules/.pnpm by default, so we need to explicitly add this dependency. See testing-library/jest-dom#123 (comment)
|
What can I do if my repo also has Chai as dependency, and the global |
|
|
That doesn't include the extended |
|
At a party atm but you can globalise a constant |
|
Seems like this is still broken when using import '@testing-library/jest-dom';
import { expect, test } from '@jest/globals';
import { render, screen } from '@testing-library/react';
import App from './App';
test('renders learn react link', () => {
render(<App />);
const linkElement = screen.getByText(/learn react/i);
expect(linkElement).toBeInTheDocument(); // 💥 Property 'toBeInTheDocument' does not exist on type
// 'Matchers<void, HTMLElement> & SnapshotMatchers<void,
// HTMLElement> & Inverse<JestMatchers<void, HTMLElement>> &
// PromiseMatchers<...>'
});I've been trying to find alternatives, including interesting overrides of Another option was the comment by @mrazauskas here:
But neither seem to offer a way to use |
|
I guess provide the same functional for users who declare module 'expect' {
interface AsymmetricMatchers {
// ...
}
interface Matchers<R> {
// ...
}
}The build-in |
|
Interesting, I did already try that and it didn't work before. But now in trying to create a reproduction for it, I cannot make it fail in a TypeScript Playground (see the working playground below) 😄 I'll try to reproduce in my app. TypeScript Playground (working) import { expect, test } from '@jest/globals';
declare module 'expect' {
interface AsymmetricMatchers {
}
interface Matchers<R> {
/**
* @description
* Assert whether an element is present in the document or not.
* @example
* <svg data-testid="svg-element"></svg>
*
* expect(queryByTestId('svg-element')).toBeInTheDocument()
* expect(queryByTestId('does-not-exist')).not.toBeInTheDocument()
* @see
* [testing-library/jest-dom#tobeinthedocument](https://github.com/testing-library/jest-dom#tobeinthedocument)
*/
toBeInTheDocument(): R;
}
}
test('renders learn react link', () => {
expect(1).toBe(1);
expect('').toBeInTheDocument(); // ✅ Property 'toBeInTheDocument' from Matchers interface above
}); |
|
Ok, so I got it to break again across file boundaries: in the same file it was working as above, but as soon as I moved my
|
Workaround (using
|
|
Thanks @mrazauskas for the hint and the push to try out that option again!! 🙌🚀 |
|
Thanks for the details. Somehow I missed that Perhaps If Might work, but it might be that Just to say that I don’t use - import type { JestExpect } from '@jest/expect';
+ import type { JestExpect, Matchers as JestMatchers } from '@jest/expect'; declare namespace jest {
+ export type Matchers<R extends void | Promise<void>, T> = JestMatchers<R, T>; |
|
It was interesting idea, but it does not work. No luck with that. In the other hand, if I have declare module 'expect' {
interface Matchers<R = void> extends TestingLibraryMatchers<typeof expect.stringContaining, R> {}
}EDIT If declare module '@jest/globals' {
interface Matchers<R = void> extends TestingLibraryMatchers<typeof expect.stringContaining, R> {}
} |
|
Nice, cool, so if I understand correctly, there are two ways that this could be resolved for the combination of
Did I understand that correctly? Would be of course great to have it work out of the box and not have to fix this from userland...! |
|
Thanks for the detailed posts @karlhorky! The snippet you shared (posted again below) worked for me using npm. import { expect } from '@jest/globals';
import { TestingLibraryMatchers } from '@testing-library/jest-dom/matchers';
declare module 'expect' {
interface Matchers<R = void>
extends TestingLibraryMatchers<typeof expect.stringContaining, R> {}
} |
Until we switch from Jest to Vitest Ref: testing-library/jest-dom#123 (comment)
Until we switch from Jest to Vitest Ref: testing-library/jest-dom#123 (comment)
and others
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:
testing-library__jest-domtoDefinitelyTyped@types/testing-library__jest-domto@testing-library/jest-domdependenciesI 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?
The text was updated successfully, but these errors were encountered: