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

Vitest #114

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Vitest #114

wants to merge 10 commits into from

Conversation

oscarleonnogales
Copy link

Quick PR Summary

  • Setup the base config to support vitest
  • Removed jest support
  • Moved all type declarations into types file

Why

Vitest is a "blazingly fast" unit test framework, and allows us to leave the test suite running in a watch-mode environment while we continue to make changes to our code. Only the necessary tests will re-run depending on what files were changed, making it much quicker to make changes without having to wait for the entire test suite to re-run.

Vitest has also been designed with a Jest compatible API, in order to make the migration from Jest as simple as possible. The API is essentially the same as Jest with some very minor differences.

@oscarleonnogales oscarleonnogales requested a review from a team as a code owner June 2, 2023 21:37
Copy link
Contributor

@hollabaq86 hollabaq86 left a comment

Choose a reason for hiding this comment

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

Great stuff! Excited for this work

@@ -2,3 +2,46 @@ export type Verification = {
isValid: boolean;
isPotentiallyValid: boolean;
};

export type CreditCardType = {
Copy link
Contributor

Choose a reason for hiding this comment

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

great catch on these

package.json Show resolved Hide resolved
Copy link
Contributor

@jplukarski jplukarski left a comment

Choose a reason for hiding this comment

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

A couple questions, and it looks like the tests are failing for what might be a Common.js issue?

Great stuff!

@@ -1,5 +1,6 @@
{
"compilerOptions": {
"moduleResolution": "nodenext",
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious, what does this do?

Copy link
Author

@oscarleonnogales oscarleonnogales Jun 16, 2023

Choose a reason for hiding this comment

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

It instructs TypeScript to use Node.js-style module resolution for the tests.

Changed to "node"

month: string | null;
year: string | null;
}
import type { ExpirationDateVerification } from "./types";
Copy link
Contributor

Choose a reason for hiding this comment

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

Love the renaming of this variable

Copy link
Contributor

Choose a reason for hiding this comment

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

is this rename a breaking change?

@@ -19,4 +19,4 @@ const cardValidator = {
postalCode,
};

export = cardValidator;
export default cardValidator;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does changing this to have a default export change the way consumers of this module import it?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, you can see the change in the __tests__/credit-card-type.ts file

Before: import cardValidator = require("../")
After:import cardValidator from "../"

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it! Would this be a breaking change?

Copy link
Contributor

Choose a reason for hiding this comment

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

same concern here, would this be a breaking change?

@ibooker
Copy link
Contributor

ibooker commented Jun 16, 2023

A couple questions, and it looks like the tests are failing for what might be a Common.js issue?

Great stuff!

https://www.typescriptlang.org/tsconfig#moduleResolution

'node16' or 'nodenext' for Node.js’ ECMAScript Module Support from TypeScript 4.7 onwards

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.

4 participants