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(clerk-react,nextjs): Type checking in vitest unit tests #4844

Conversation

panteliselef
Copy link
Member

@panteliselef panteliselef commented Jan 7, 2025

Description

We need to opt in to typechecking in order to the type errors throw by expect-type to be caught and cause the tests to fail.

Checklist

  • pnpm test runs as expected.
  • pnpm build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

Copy link

changeset-bot bot commented Jan 7, 2025

🦋 Changeset detected

Latest commit: 7cfb605

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 0 packages

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

vercel bot commented Jan 7, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
clerk-js-sandbox ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 7, 2025 5:42pm

@panteliselef panteliselef changed the title fix(repo): Type checking in vitest unit tests fix(clerk-react,nextjs): Type checking in vitest unit tests Jan 7, 2025
@panteliselef panteliselef requested a review from a team January 7, 2025 13:30
@panteliselef panteliselef self-assigned this Jan 7, 2025
@panteliselef panteliselef marked this pull request as ready for review January 7, 2025 13:35
@panteliselef panteliselef reopened this Jan 7, 2025
@panteliselef panteliselef reopened this Jan 7, 2025
Comment on lines +188 to +190
if (!current.userId) {
throw 'Invalid state';
}
Copy link
Member

Choose a reason for hiding this comment

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

Let's just assert this is truthy

Suggested change
if (!current.userId) {
throw 'Invalid state';
}
expect(current.userId).toBeTruthy();

Copy link
Member Author

Choose a reason for hiding this comment

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

I did this in order to have typescript narrow down the type of current.has below and make not undefined.

Copy link
Member

Choose a reason for hiding this comment

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

It's not clear that's why this logic is here then. Why not just use optional chaining below

const result = current.has?.({ permission: 'test' });


const result = current.has({ permission: 'test' });
expect(result).toBe(false);
expectTypeOf(result).toBeBoolean();
Copy link
Member

Choose a reason for hiding this comment

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

This type assertion isn't really needed as you are already asserting that result is strictly false

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted this expectTypeOf(result).toBeBoolean(); which performs a type check. I can simply revert tbh.

@jacekradko
Copy link
Member

@panteliselef 👍🏼 This makes sense. I noticed a while back that we have some blind spots as far as type checks go so started looking at it here: #4778
Some combination of these 2 PRs should have us in a much better place. @brkalow thoughts?

@panteliselef
Copy link
Member Author

panteliselef commented Jan 7, 2025

I went with this route since we are using expect-type already. And that is included in vitest.

Mostly I wanted to ensure that any testing around types that previously was working with jest, continues to do so.

On the long run I'm happy to change or keep it.

@jacekradko
Copy link
Member

@panteliselef I agree this is the right way to go 💯 , especially for those expectTypeOf tests. To be honest, it's kind of annoying that expect-type assertions pass even though they shouldn't be since they don't have access to type information without this change 🤷🏼

Copy link
Member

@jacekradko jacekradko left a comment

Choose a reason for hiding this comment

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

:shipit:

@panteliselef panteliselef merged commit 22ace70 into main Jan 8, 2025
29 checks passed
@panteliselef panteliselef deleted the elef/sdki-820-type-checking-not-working-on-vitest-unit-tests branch January 8, 2025 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants