-
Notifications
You must be signed in to change notification settings - Fork 84
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
Migrated to vitest #571
Migrated to vitest #571
Conversation
|
] | ||
|
||
type ExpecteResult = Result<[ string, number, boolean ], (string | number | boolean)[]> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like prettier formatting
"strictNullChecks": true, | ||
"strictFunctionTypes": true, | ||
"declaration": true, | ||
"skipLibCheck": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only change here is adding skipLibCheck: true to remove an erroneous error when I upgraded
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was curious to know what the issues were. Here they are (at least for posterity):
... interestingly WeakKey
is infact a ts builtin:
As for the "Cannot find module 'rollup/parseAst' or its corresponding type declarations." issue, I came across this thread:
Any thoughts @dmmulroy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WeakKey
is available in TS >= 5.2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Errors thrown in node_modules
don't matter - it's not code that we control. If there are no errors on install (i.e. peerDeps are all good) then we're happy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm - thanks Matt
@m-shaka @supermacro I am much in favor of this change and have been using vitest
by default for a year or so now
…ck/neverthrow into matt/migrated-to-vitest
Love this. Thank you 🙌 |
@supermacro Getting the same error as here on this PR |
@mattpocock @m-shaka Can we get tests/CI passing and get this in? |
Too many conflicts, easier to just create a new branch. See #597 |
Fewer dependencies, more modern tooling, in the same spirit as #570