search

testing-library / jest-dom

:owl: Custom jest matchers to test the state of the DOM
https://testing-library.com/docs/ecosystem-jest-dom
MIT License
4.45k stars 401 forks source link

Types breakage in v6.4.0 #574

Closed Rugvip closed 9 months ago

Rugvip commented 9 months ago

We're seeing an issue with the types in the latest release (v6.4.0). In particular the addition of the ByRoleMatcher type declaration breaks the export assignment later on, resulting in the following error:

TS2309: An export assignment cannot be used in a module with other exported elements.

Relevant code or config:

tsconfig.json

What you did:

Example breakage in a CI run here: https://github.com/backstage/backstage/actions/runs/7708717042/job/21008397598?pr=22596

What happened:

node_modules/@testing-library/jest-dom/types/matchers.d.ts:713:1 - error TS2309: An export assignment cannot be used in a module with other exported elements.

713 export = matchers
    ~~~~~~~~~~~~~~~~~

Found 1 error in node_modules/@testing-library/jest-dom/types/matchers.d.ts:713

Reproduction:

Doesn't seem to reproduce in codesandbox, may be related to TypeScript version or config?

Problem description:

Breaks TypeScript type checks.

Suggested solution:

Might be enough to switch the re-export to export { matchers }?

fpapado commented 9 months ago

Hi @Rugvip! I am not a maintainer, but I am the person that contributed .toHaveRole, so I take some of the responsibility 😌 First of all, my apologies for the hassle!

Two quick checks

Can I ask you to check two things on your end?

  1. If you modify matchers.d.ts to remove the export declaration for ByRoleMatcher, are the type errors fixed?
  2. If not, can you check removing the import and export for ByRoleMatcher, and changing this line to role: string?

Since the core export of the package is the matchers, there is no need to separately export the ByRoleMatcher anyway. We just want the argument to .toHaveRole to be well-typed (the full type of the matcher can be extracted with Parameters/ReturnType and co if need be, I suppose) So option 1 might be a simple way to thread the needle, and a follow-up PR :)

You can use patch-package, pnpm's native patching, or even the code editor in a pinch, whichever you prefer.

I ask you, because I tried to reproduce this issue in our codebase at work, and we are not getting this issue. Tests type-check both with and without the change in option 1 above. TypeScript config is vast, so I'm not drawing any conclusions yet 😅

Some more context on the fixes

Might be enough to switch the re-export to export { matchers }?

This would probably not be enough. For example, the type tests in this repository fail with that change.

You can run the type tests in the repository via npm run test:types. The type tests themselves are in the types/tests directory.

I tried copying over your tsconfig.json into that tests folder, and the tests seemed to still pass. At a glance, I don't see anything in the config that could cause this, so I suspect some other underlying cause.

Future testing

I have a gut feeling that the type tests in this repository are missing some use-cases (perhaps config combinations), if they can break like this. I don't have a concrete suggestion for it right now; maybe the maintainers have run into this before?

Rugvip commented 9 months ago

@fpapado Thank you for the reply! 🎉

  1. Yes, if I switch it to just type ByRoleMatcher = ARIARole | (string & {}) I no longer see any errors.

I played around with the types a bit in this repo. I didn't find any instructions for how to check types, but I've been running npm exec tsc. Something seems strange with the setup in this repo. If I for example add something obviously broken like export 'bad' at the end of matchers.d.ts, I get an error. But even something like switching export = matchers to export = nonexistent doesn't break the type checking.

I moved over to reproducing in a minimal Backstage setup instead:

npx @backstage/create-app@latest --skip-install # "repro-jest-dom" as app name
cd repro-jest-dom
yarn install
yarn tsc:full # error

In that setup I could verify that removing the ByRoleMatcher export at least make those type checks happy.

fpapado commented 9 months ago

Thanks for checking, @Rugvip!

I opened a PR for the maintainers to consider, but I do not have a good test case to add to the repo. So I leave it up to them, how to proceed. I opened the PR mostly to account for any timezone differences, not because it is the end-all-be-all solution :)

But even something like switching export = matchers to export = nonexistent doesn't break the type checking.

Did you try running the type tests with npm run test:types? There's different tsconfig projects under types, so they are run individually, via that script. When I do that with a nonsensical export = notAThing, they seem to error as intended:

https://github.com/testing-library/jest-dom/assets/3210764/74446c2e-d399-452e-abd0-19d58ef4fcf0

Rugvip commented 9 months ago

@fpapado yep those do catch the export = notAThing, but as far as I can tell it's because the matchers are of the things explicitly tested. If you for example add type non = existent at the bottom that's not caught.

fpapado commented 9 months ago

Aha! I believe it is "skipLibCheck": true in the root tsconfig.json that is causing these errors to be silently ignored.

skipLibCheck skips checking all .d.ts files, unless specifically referenced by the source code. This explains the discrepancy that we are seeing between tests and running tsc directly 💡

By switching it off, or by running tsc directly on the types/matchers.d.ts file (i.e. bypassing tsconfig.json), the original error shows up, without the changes in #575.

However, when changing this, there is a cascade of other errors that show up. I do not know which ones are valid, because the type overloading / declaration merging in this library seems complex. This leads me to think that skipLibCheck is somehow load-bearing for the type setup here, but at least this gives a way to verify the changes!

Incidentally, at work, we also have "skipLibCheck": true, which would explain why I could not reproduce this originally.

https://github.com/testing-library/jest-dom/assets/3210764/d35e982b-cbb9-4d1d-ac5b-f385277ba94b

SantoJambit commented 9 months ago

FYI, using skipLibCheck will cause type-errors to silently result in any types, so it's not a good idea to set this unless you really really need to.

fpapado commented 9 months ago

@Rugvip, the fix to this should now be released as part of 6.4.2 👀 Can you check on your end? :)

pjungermann commented 9 months ago

I tried it in our setup, and the fix works. Thank you!

Rugvip commented 9 months ago

Works for us too, thank you! 👍