-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
refactor: use vite-node to load globalSetup #624
Conversation
✔️ Deploy Preview for vitest-dev ready! 🔨 Explore the source changes: b8752ea 🔍 Inspect the deploy log: https://app.netlify.com/sites/vitest-dev/deploys/61f04f70b0837e0008ab6026 😎 Browse the preview: https://deploy-preview-624--vitest-dev.netlify.app |
addition of .gitattributes and .editorconfig was done to enforce LF line endings on all platforms. see vitejs/vite#5092 |
@@ -8,12 +10,23 @@ interface GlobalSetupFile { | |||
} | |||
|
|||
async function loadGlobalSetupFiles(server: ViteDevServer): Promise<GlobalSetupFile[]> { | |||
const node = new ViteNodeServer(server) |
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.
can't you reuse vitenode
from ctx
? it relies on user config
- https://github.com/vitest-dev/vitest/blob/main/packages/vitest/src/node/plugins/index.ts#L9
- https://github.com/vitest-dev/vitest/blob/main/packages/vitest/src/node/core.ts#L84
I think it's undefined at the initialization, but you can pass the whole context to GlobalPlugin
, I think
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.
loadGlobalSetupFiles is called in buildStart hook, so undefined at init wouldn't be an issue.
globalSetupFiles = await loadGlobalSetupFiles(server) |
i'm not sure how to get/pass ctx into GlobalPlugin though, can you elaborate on that?
got the code to init the ViteNodeRunner from here
vitest/packages/vite-node/src/cli.ts
Line 77 in 568668a
const node = new ViteNodeServer(server) |
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.
Is that possible to run the global setup after the vite server starts? Or what will that cause?
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.
globalSetup should run once before any testfile is executed and teardown once after all testfiles are done. As long as that is true, it's ok to not execute it on vite server start or even before (that would make it impossible to use the vite server for loading the files anyways).
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.
loadGlobalSetupFiles is called in buildStart hook, so undefined at init wouldn't be an issue.
globalSetupFiles = await loadGlobalSetupFiles(server) i'm not sure how to get/pass ctx into GlobalPlugin though, can you elaborate on that?
got the code to init the ViteNodeRunner from here
vitest/packages/vite-node/src/cli.ts
Line 77 in 568668a
const node = new ViteNodeServer(server)
You can pass ctx to your plugin when initializing VitestPlugins:
export async function VitestPlugin(options: UserConfig = {}, viteOverrides: ViteUserConfig = {}, ctx = new Vitest()): Promise<VitePlugin[]> { |
GlobalSetupPlugin(), |
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.
ahh, yes that works although it only saves a little bit of code because the runner would still have to be inizialized. Maybe it is even preferable to use a separate instance to avoid globalSetup polluting the main instance?
On the flipside maybe we could/should pass the ctx down into setup so that it can be used in there and also maybe as a possible backchannel to add something to test contexts?
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.
ahh, yes that works although it only saves a little bit of code because the runner would still have to be inizialized. Maybe it is even preferable to use a separate instance to avoid globalSetup polluting the main instance?
On the flipside maybe we could/should pass the ctx down into setup so that it can be used in there and also maybe as a possible backchannel to add something to test contexts?
Well, it at least should use test config, I think. Or otherwise deps
and transformMode
will have no effect in user config.
vitest/packages/vitest/src/node/core.ts
Line 84 in de33284
this.vitenode = new ViteNodeServer(server, this.config) |
const runner = new ViteNodeRunner({ | ||
root: server.config.root, | ||
base: server.config.base, | ||
fetchModule(id) { | ||
return node.fetchModule(id) | ||
}, | ||
resolveId(id, importer) { | ||
return node.resolveId(id, importer) | ||
}, | ||
}) |
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.
Could we also reuse the vite-node server?
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.
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.
Ah, never mind, I misread :P
and add test launching a vite instance
suggested by @antfu on discord
related: #522