-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat: Pass through vite configLoader option to createViteServer #7574
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
feat: Pass through vite configLoader option to createViteServer #7574
Conversation
This exposes the option to the CLI, passes it to `ViteInlineConfig`s where needed and adds some docstrings to InlineConfig etc.
✅ Deploy Preview for vitest-dev ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -160,6 +160,7 @@ export function resolveConfig( | |||
} | |||
|
|||
resolved.clearScreen = resolved.clearScreen ?? viteConfig.clearScreen ?? true | |||
resolved.configLoader = resolved.configLoader ?? viteConfig.inlineConfig.configLoader ?? 'bundle' |
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 am actually unsure about this part. Not sure if this is needed?
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.
As I commented in #7574 (review), configLoader
is something meaningful before resolving config, so it's likely we don't need to care about and thus should be removed.
Hey, thanks for the PR 👋 Not sure if it's intentional, but why close? If you want to contribute, opening a PR here is correct instead of on your fork janis-me#1. |
Hey! I closed the one to vitest, because I didn't write any tests yet and just wanted to show my "idea" first. But I can re-open It with tests if you are mostly fine with the implementation |
Okay, don't worry about leaving an unfinished PR here. You can make it as draft and also fine to ask for the feedback through comments. 👍 |
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.
Typing might be a bit tricky. We don't want UserConfig.configLoader
to be available since that means it's allowing defineConfig({ test: { configLoader: "..." } })
, but that doesn't work since configLoader
is used to load the config.
We probably want to add only CliOptions.configLoader
and typing needs to be tweaked on createVitest
and initializeProject
side.
export interface CliOptions extends UserConfig { |
Also, configLoader
is only available for certain Vite version, so it would be nice if we can detect that by infering the type from import("vite").InlineConfig["configLoader"]
or something.
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 pushed a simple test case.
@@ -160,6 +160,7 @@ export function resolveConfig( | |||
} | |||
|
|||
resolved.clearScreen = resolved.clearScreen ?? viteConfig.clearScreen ?? true | |||
resolved.configLoader = resolved.configLoader ?? viteConfig.inlineConfig.configLoader ?? 'bundle' |
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.
As I commented in #7574 (review), configLoader
is something meaningful before resolving config, so it's likely we don't need to care about and thus should be removed.
configLoader: { | ||
description: | ||
'Use `bundle` to bundle the config with esbuild or `runner` (experimental) to process it on the fly (default: `bundle`)', | ||
argument: '<loader>', | ||
}, |
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.
documentation should also mention this is only available from newer Vite version.
Before, it was defined in the `InlineConfig` and thus `UserConfig` interfaces, which would not have worked when the option was passed to e.g `defineConfig(...)`
…` and `configLoader` option
Good catch! I updated the PR to only include I also noticed, that in Also, thanks for adding the test! I will extend on these to also test more 'complex' configurations. |
The issue here is, that the vitest/packages/vitest/src/node/workspace/resolveWorkspace.ts Lines 75 to 79 in 1a88b1b
So would I need to add vitest/packages/vitest/src/node/workspace/resolveWorkspace.ts Lines 30 to 35 in 1a88b1b
I might not be qualified for working on this feature after all xD |
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.
Thanks for looking into it 👍 I simplified a bit by passing vite.config.inlineConfig.configLoader
to child vite servers, so we don't need to track the config on our typing, but this should essentially give the desired result.
Thanks a lot for carrying me through this. Is there something left I can help with? It seems like the test is failing for me, I can investigate why that might be. |
@vitest/browser
@vitest/coverage-istanbul
@vitest/expect
@vitest/coverage-v8
@vitest/mocker
@vitest/pretty-format
@vitest/runner
@vitest/snapshot
@vitest/spy
@vitest/ui
@vitest/utils
vite-node
@vitest/web-worker
vitest
@vitest/ws-client
commit: |
This will be merged when releasing next minor 3.1. Tests passed on CI, so maybe your local setup issue? If you want to verify if this change works on your project, you may try pkg-pr-new #7574 (comment). |
Can you resolve merge conflicts? |
Done. Note that the lockfile change only consists of the lines
which is expected. See 59630b5 for the commit. Hope it looks okay |
Description
(this is a draft to verify the implementation. It lacks tests)
This PR
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
pnpm-lock.yaml
unless you introduce a new test example.Tests
pnpm test:ci
.Documentation
pnpm run docs
command.Changesets
feat:
,fix:
,perf:
,docs:
, orchore:
.