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

Move Types from @types/* to this repo #314

Closed
MatanBobi opened this issue Dec 1, 2020 · 14 comments · Fixed by #511
Closed

Move Types from @types/* to this repo #314

MatanBobi opened this issue Dec 1, 2020 · 14 comments · Fixed by #511
Assignees
Labels

Comments

@MatanBobi
Copy link
Member

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:
testing-library/dom-testing-library#530

@MatanBobi MatanBobi self-assigned this Dec 1, 2020
@gnapse
Copy link
Member

gnapse commented Dec 1, 2020

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.

@MatanBobi
Copy link
Member Author

MatanBobi commented Dec 1, 2020

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 :)

@gnapse
Copy link
Member

gnapse commented Jun 11, 2021

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
Copy link
Member

gnapse commented Jun 11, 2021

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

@eps1lon
Copy link
Member

eps1lon commented Jun 11, 2021

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

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

@gnapse
Copy link
Member

gnapse commented Jun 11, 2021

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 😄)

@jdanil
Copy link

jdanil commented Oct 30, 2021

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?

@gnapse
Copy link
Member

gnapse commented Nov 1, 2021

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.

@jdanil
Copy link

jdanil commented Nov 2, 2021

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.

@ph-fritsche
Copy link
Member

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.

@gnapse
Copy link
Member

gnapse commented Nov 2, 2021

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.

@nickserv
Copy link
Member

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.

@IanVS
Copy link
Contributor

IanVS commented Oct 6, 2022

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...

@github-actions
Copy link

🎉 This issue has been resolved in version 6.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants