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

feat(tools): enable typecheck on all TypeScript files #97

Merged
merged 6 commits into from
Apr 25, 2024

Conversation

huyenltnguyen
Copy link
Member

@huyenltnguyen huyenltnguyen commented Apr 24, 2024

Checklist:

I noticed that if we have a TS error in a test or a storybook file, pnpm run typecheck would ignore them.

This PR:

  • Updates the TypeScript and Rollup config to enable typecheck on all TypeScript files, while still keeping irrelevant types out of the package bundle.
  • Converts Jest config and setup files to TypeScript - This is a change I've been wanting to make. I didn't intend to include it in this PR, but I had to in order to run typecheck on test files (details in the PR comments)
  • Adds back ts-node - It is unfortunately a buddy of Jest (ref)
    • Screenshot 2024-04-24 at 23 07 03
  • Removes tsx

Testing

  • Make a TS error in a *.test.tsx or a *.storybook.tsx file
  • Run pnpm run typecheck and check if it throws an error
  • Run pnpm run build and check if types in test and storybook files are included in the bundle

@@ -0,0 +1,8 @@
import type { Config } from "jest";
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the file following https://jestjs.io/docs/configuration.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can read how I went through the rabbit hole below, but here is the TL;DR version: The tsc command threw a lot of errors when checking test files. This was because it couldn't find the type definitions of @testing-library/jest-dom. Since @testing-library/jest-dom is imported into this file, I reckoned tsc couldn't see the types simply because it ignores JS files, so the fix would be converting the file to TS.


Okay, so I think the issue started from #85 when I removed @types/testing-library__jest-dom.

The package was added during the Storybook 8 upgrade to resolve some TS errors. I later found that this is an internal package of @testing-library/jest-dom, and we aren't supposed so consume it directly as @testing-library/jest-dom already includes type definitions, so I removed the types package.

Upon removing the package, the editor showed me a lot of TS errors in test files because apparently TS couldn't find the type definitions for the matchers. Upgrading @testing-library/jest-dom resolved the issue for me (we didn't have typecheck run on test files, so the tsc command didn't error out).

With typecheck being enabled on all TS files, the errors re-surfaced (my editor didn't have any errors, but tsc was clearly not happy).

There are a bunch of similar issues on the @testing-library/jest-dom repo:

#442 has a lot of details, and here is the summary:

  • The way the package exposes type definitions appears to be unconventional, and TypeScript couldn't pull the types out
  • Some workarounds are:
  • pnpm doesn't hoist @types packages by default. The fix is to create a .npmrc file and specify public-hoist-pattern.
    • There is a note from the public-hoist-pattern doc: "This setting is useful when dealing with some flawed pluggable tools that don't resolve dependencies properly."
    • The public-hoist-pattern change seemed to work for some people, but not for me.
    • I found this pnpm behavior interesting but didn't spend much time on it, as I don't think we should mess with the pnpm settings just to get a weird package to work.

That was it from the issue thread. And it took me a looong while to come up with a theory:

  • @testing-library/jest-dom contains the type definitions
  • The only place that imports @testing-library/jest-dom is jest.setup.js
  • Since jest.setup.js is a JS file, tsc ignores it and thus doesn't pull out the types it needs

The next thing I did was converting jest.setup.js and jest.config.js to TS. However, the test command didn't work because Jest requires ts-node, which we recently swapped for tsx. This is why I had to add ts-node back in and remove tsx.

@huyenltnguyen huyenltnguyen marked this pull request as ready for review April 24, 2024 18:02
@huyenltnguyen huyenltnguyen requested a review from a team as a code owner April 24, 2024 18:02
@huyenltnguyen
Copy link
Member Author

cc'ing @ojeytonwilliams since I'm undoing your changes here 😄

Copy link
Contributor

@ojeytonwilliams ojeytonwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Impressive detective work, @huyenltnguyen! Thanks for the detailed write-up.

Based on your investigations I found another workaround: add allowJs: true to the tsconfig. That way, jest-setup.js's type imports get compiled and TS is happy.

My only concern with the approach in this PR is ts-node, because it's a pain when trying to use ESM. However, we can either use allowJs or simply use ts-node for jest and tsx to run scripts.

Anyways, none of that matter for this PR!

LGTM 👍

@ojeytonwilliams ojeytonwilliams merged commit d6d6ad5 into freeCodeCamp:main Apr 25, 2024
5 checks passed
@huyenltnguyen huyenltnguyen deleted the feat/typecheck branch April 25, 2024 15:38
@huyenltnguyen
Copy link
Member Author

because it's a pain when trying to use ESM

Oh, TIL. I wasn't aware that ts-node doesn't work well with ESM 😅 I can create a PR to add tsx back.

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

Successfully merging this pull request may close these issues.

None yet

3 participants