jest-dom icon indicating copy to clipboard operation
jest-dom copied to clipboard

Move Types from @types/* to this repo

Open MatanBobi opened this issue 3 years ago • 12 comments

Describe the feature you'd like:

Following this comment and this issue, we would like to move the types to this repo also.

Suggested implementation:

Here's a PR done in dom-testing-library: https://github.com/testing-library/dom-testing-library/pull/530

MatanBobi avatar Dec 01 '20 19:12 MatanBobi

Usually there have been more difficulty with our repo than with other libs in the testing-library org because unlike other libs, this one is not used via direct imports. That usually has been a blocker.

For instance, we used to have the types baked in, and we had to extract them because it did not work properly with VSCode intellisense. See #123 for instance. There was even an effort to port the actual source code to TypeScript (see #246) and it too introduced some issues that made it not suitable for a release.

That being said, I'd be glad if we can pull it off provided it is done so that all the previous issues that we've had with this would not come back.

gnapse avatar Dec 01 '20 20:12 gnapse

Usually there have been more difficulty with our repo than with other libs in the testing-library org because unlike other libs, this one is not used via direct imports. That usually has been a blocker.

For instance, we used to have the types baked in, and we had to extract them because it did not work properly with VSCode intellisense. See #123 for instance. There was even an effort to port the actual source code to TypeScript (see #246) and it too introduced some issues that made it not suitable for a release.

That being said, I'd be glad if we can pull it off provided it is done so that all the previous issues that we've had with this would not come back.

Thanks @gnapse, I wasn't aware of the previous attempts that led to us moving this to Definitely-Typed. I'll give it a shot and update here :)

MatanBobi avatar Dec 01 '20 20:12 MatanBobi

I gotta say, the barrier of entry to contribute types via having to do it in DefinitelyTyped/DefinitelyTyped is huge. I love TS, and I can barely drag myself to do it (cloning that huge repo or following the instructions to clone sparsely is discouraging). I really wish we could do this somehow. I will take a look at how other jest matchers libraries do it.

gnapse avatar Jun 11 '21 15:06 gnapse

cc @testing-library/all-maintainers in case someone has some ideas on how to pull this off.

gnapse avatar Jun 11 '21 15:06 gnapse

If I remember correctly there wasn't much to it.

  1. Add types to the original repository (SemVer minor)
  2. Remove types from DefinitelyTyped

eps1lon avatar Jun 11 '21 16:06 eps1lon

We may attempt it again, but the main reason we moved the type definitions from being inside the repo was #123.

There were also issues with actually writing the code in TS. See #246 (which funnily enough is 123 * 2 😄)

gnapse avatar Jun 11 '21 16:06 gnapse

I think another reason in favour of this proposal would be compatibility with stricter node linkers. I'm finding that when using @testing-library/jest-dom with pnp or pnpm node linkers, the type definitions declared in this package don't actually work because they are not a dependency of the consuming project.

For example, the node modules structure may look something like this...

my-project
  node_modules
    @testing-library
      jest-dom
        node_modules
          @types
            testing-library__jest-dom

So my-project can't actually resolve @types/testing-library__jest-dom as it would just look at its node_modules directory and its parent node_modules directories, not the nested directories. The current behaviour is relying on package managers to flatten dependencies so that they can be accessed implicitly, but that can lead to unsafe access of undeclared dependencies.

In these cases I am having to add @types/testing-library__jest-dom to my project anyway.

In contrast, compared to other extensions to jest matchers like jest-extended, because the types are exported by the same package, they are correctly resolved by the project.

I'd be happy to submit a PR to implement @eps1lon's suggestion if everyone is happy with that approach?

jdanil avatar Oct 30 '21 00:10 jdanil

It'd be awesome if you do that @jdanil.

Can you please also keep in mind the difficulties we found when previously attempting this in #246? Although in that case it was different: the actual code was migrated to TS. Maybe having just type definitions and keeping the code in JS is sufficiently different to not cause any troubles.

gnapse avatar Nov 01 '21 13:11 gnapse

Ok I had a bit of a play around today. I think if the types are moved to this package, then the issue in #246 is unavoidable (that you need to explicitly import @testing-library/jest-dom for the types to be available). That means you can't simply add "setupFilesAfterEnv": ["@testing-library/jest-dom"]. You need to either create some setup.ts file and import @testing-library/jest-dom there, or you can still include it in setupFilesAfterEnv but you will also need to add a custom.d.ts file that imports @testing-library/jest-dom to add the types. jest-extended also instructs typescript users to setup their configuration this way.

An alternative I was playing around with that doesn't necessarily resolve this issue of moving the type definitions, but avoids the need to add @types/testing-library__jest-dom as a dependency for users using stricter node linkers was to add a "types": "@types/testing-library__jest-dom" entry to the package.json. So @testing-library/jest-dom exposes the types from DefinitelyTyped. I haven't seen this approach in the wild before, but it worked in my local testing.

jdanil avatar Nov 02 '21 06:11 jdanil

I think the way this should be solved is a declaration file in this repo that imports the matcher types from the modules (might use declaration files as a stepping stone while converting modules to TS) and augments the Matchers in jest namespace.

From there it should be a dual approach:

  1. For those without the need of extra configuration there should be a @types/testing-library__jest-dom package. a) I think we could feed the DefinitelyTyped per GitHub Action. b) The declaration could import types from @testing-library/jest-dom so that it automatically reflects the installed version.
  2. Everybody else could either add @types/testing-library__jest-dom as dependency or add the declaration folder from @testing-library/jest-dom to the typeRoots.

ph-fritsche avatar Nov 02 '21 07:11 ph-fritsche

Can you go ahead with a PR? We could do the same we did with #246, publishing a beta or alpha release to see if it all works as expected when this gets published.

gnapse avatar Nov 02 '21 17:11 gnapse

Couldn't we solve this problem by recommending to use /// <reference ... /> comments in test setup files? create-react-app, Vite, and Vitest do something similar.

nickmccurdy avatar Sep 21 '22 07:09 nickmccurdy

Or adding to the tsconfig.json "types" field is another approach that is often used as well. I'd really love to get the type story straightened out here. Storybook is trying to update our version of expect to 28, and the module augmentation in jest-dom is breaking. It all seems very fragile...

IanVS avatar Oct 06 '22 14:10 IanVS

:tada: This issue has been resolved in version 6.0.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

github-actions[bot] avatar Aug 13 '23 16:08 github-actions[bot]