-
Notifications
You must be signed in to change notification settings - Fork 209
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
Improve vitest config and add GitHub action for lint + testing #22
Conversation
Yes I favor this too, but just haven't taken the time to set it up yet. We should do it in the not too distant future. |
@@ -4,7 +4,7 @@ import path from 'node:path'; | |||
|
|||
export const SRCBOOK_DIR = path.join(os.homedir(), '.srcbook'); | |||
|
|||
const userDir = import.meta.env.VITE_NOTEBOOKS_DIR || os.homedir(); |
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 think we should re-add this. Idk that it should affect anything here
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.
we should readd this as process.env.MY_ENV_VAR
.
I don't know why the vite stuff and vite namespace is used for regular operating system environment variables. The vite specific stuff is for building frontend apps that are compiled for the browser.
b3cd852
to
04b65f5
Compare
5205264
to
0a48398
Compare
0a48398
to
5205264
Compare
5205264
to
1f4ed6f
Compare
Closing this as its way out of date. #52 introduces monorepo, so we'll want to build on top of that. |
I followed the advice on the vitest getting started.
One thing I considered, but didn't do, is separate our packages into a monorepo, so
If we did this, we could leverage vitest workspaces which map nicely to npm's testing. I don't think it's a huge gain so didn't do it, but I slightly favor it in the future.