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

fix: vitest typecheck #245

Merged
merged 2 commits into from
Sep 21, 2024
Merged

Conversation

yamcodes
Copy link
Contributor

@yamcodes yamcodes commented Sep 14, 2024

Tip

The owner of this PR can publish a preview release by commenting /publish in this PR. Afterwards, anyone can try it out by running pnpm add radashi@pr<PR_NUMBER>.

Summary

Fix Vitest typecheck by (1) updating Vitest (2) enabling typecheck (3) pointing typecheck to the tests tsconfig

All 3 avenues must be applied together, omitting one causes type-d.ts files either to pass blindly (false negatives) or not report at all.

Related issue, if any:

Closes #244

For any code change,

  • Related documentation has been updated, if needed
  • Related tests have been added or updated, if needed
  • Related benchmarks have been added or updated, if needed

Does this PR introduce a breaking change?

No

Bundle impact

Status File Size 1 Difference
M src/array/merge.ts 185 -25 (-12%)

Footnotes

  1. Function size includes the import dependencies of the function.

@yamcodes
Copy link
Contributor Author

yamcodes commented Sep 14, 2024

After experimenting with Vitest versions, we need at least v2.1.0-beta.6. It seems like the following PR fixed the false negatives: vitest-dev/vitest#6256.

I went with 2.1.1 in the PR, which is the latest. I checked that it does not introduce breaking changes relevant to us. The only breaking change between 2.0.5 and 2.1.1 has to do with Vitest Workspaces which we do not use.

Copy link
Contributor

@MarlonPassos-git MarlonPassos-git left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, I had seen this issue before but hadn't taken the time to fix it.

I tested it in a codespace, and it seems to work fine now, thanks.

Even though type checking is still in beta (I think that was one of the reasons why it wasn't active), it would be worth validating it in the CI stage.

@MarlonPassos-git
Copy link
Contributor

@yamcodes just one thing, fix the PR title for “fix: vitest typecheck

@MarlonPassos-git MarlonPassos-git added the maintenance An improvement to codebase maintainability label Sep 14, 2024
@yamcodes yamcodes changed the title fix: Vitest typecheck fix: vitest typecheck Sep 14, 2024
@yamcodes
Copy link
Contributor Author

yamcodes commented Sep 14, 2024

Thanks, @MarlonPassos-git, I agree. Since we have *.test-d.ts files in the branch, we should make sure they can be used.

If we’re wary to use this feature officially, let’s add a separate script in package.json to typecheck. This way you can at least know if the type tests you write work.

However, to keep things simple, I suggest testing both together.

@aleclarson aleclarson added the priority: high This needs attention soon. label Sep 17, 2024
yamcodes and others added 2 commits September 21, 2024 13:12
Fix Vitest `typecheck` by (1) updating Vitest (2) enabling `typecheck` (3) pointing typecheck to the tests tsconfig
@aleclarson
Copy link
Member

This is great 😄 Thanks a lot, @yamcodes

@aleclarson aleclarson merged commit 74cdfd3 into radashi-org:main Sep 21, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance An improvement to codebase maintainability priority: high This needs attention soon.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vitest Typecheck is not working
3 participants