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

Remove "is-email" from testing suite #1247

Merged
merged 3 commits into from
Jun 22, 2024

Conversation

yeoffrey
Copy link
Contributor

@yeoffrey yeoffrey commented Jun 21, 2024

Removing is-email from the testing suite. Its used as a dev dependency only when testing refine, and it creates some complexity because we need to type that function. Not worth it in my opinion!

Related: #1244

@yeoffrey yeoffrey marked this pull request as draft June 21, 2024 18:23
@@ -34,7 +34,6 @@
"eslint": "^8.57.0",
"eslint-config-prettier": "^9.1.0",
"eslint-plugin-prettier": "^5.1.3",
"is-email": "^1.0.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is-email is a package that validates an email. It requires using a custom type to use, and its only used when testing refiners. Removing this allows us to remove /@types.

import { string, refine } from '../../../src'

export const Struct = refine(string(), 'email', isEmail)
export const Struct = refine(string(), 'email', (value) => value.includes('@'))

export const data = 'invalid'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For example, I've simply made the test for a refiner check that there is an @ symbol in the data. Since we weren't testing whether it actually works with emails, this seems simpler and within the scope of this commit.

@yeoffrey yeoffrey marked this pull request as ready for review June 21, 2024 18:28
@arturmuller
Copy link
Collaborator

Removing the is-email package probably makes sense, just to make it less confusing. I tripped over it a couple of times myself!

All the modules under typings are a DIY way of testing TypeScript type signatures (run via the npm run test:types command). We definitely don't want to remove them, but it would probably make a lot of sense to instead use Vitest's type testing features as they are more robust in my experience.

Using Vitest would also makes it a lot clearer what is actually happening with all those files -- a major plus in my book!

@yeoffrey yeoffrey force-pushed the remove-types-in-tests branch 2 times, most recently from a6c374e to cb3bb0d Compare June 21, 2024 21:06
@yeoffrey yeoffrey changed the title Remove excessive types in testing suite Remove is-email from testing suite Jun 21, 2024
@yeoffrey yeoffrey changed the title Remove is-email from testing suite Remove "is-email" from testing suite Jun 21, 2024
@yeoffrey
Copy link
Contributor Author

Removing the is-email package probably makes sense, just to make it less confusing. I tripped over it a couple of times myself!

All the modules under typings are a DIY way of testing TypeScript type signatures (run via the npm run test:types command). We definitely don't want to remove them, but it would probably make a lot of sense to instead use Vitest's type testing features as they are more robust in my experience.

Using Vitest would also makes it a lot clearer what is actually happening with all those files -- a major plus in my book!

Cool! Will keep those then. I've refactored this PR to just contain the changes for is-email.

@arturmuller
Copy link
Collaborator

@yeoffrey FYI: I noticed that the is-email was used in the examples dir, along with some other similar helper packages. I added a small README and a custom package.json to that dir so that we can remove all of these from the root.

@arturmuller arturmuller merged commit 88563ad into ianstormtaylor:main Jun 22, 2024
3 checks passed
@yeoffrey
Copy link
Contributor Author

@yeoffrey FYI: I noticed that the is-email was used in the examples dir, along with some other similar helper packages. I added a small README and a custom package.json to that dir so that we can remove all of these from the root.

No worries at all. Thanks for doing that!

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.

2 participants