-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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: enable cwd override for vite plugin #12410
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: dec9864 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
1aad844
to
4e63a05
Compare
Hello! I encountered the same issue as you but missed your PR, so I created my own here (#12420). I have no problem keeping yours, but I found some cases you missed. Things about the analyse part and pre-rendering for example. |
I'm not sure this is needed, you can just set the cwd when invoking the scripts instead:
|
@Anthony-Jhoiro feel free to add missing parts to this PR or finish yours. I'm fine either way. @dominikg, you can find an example of the issue in #12420. |
please share an actual repo that demonstrates the problem this PR tries to solve. The linked issue has a screenshot of a directory structure but it does not show the actual package.json scripts being used or describe what the goal really is. In general for a change of this significance i would prefer creating an issue/feature request first instead of just sending a PR that could be the solution but also just a workaround. Sveltekit deliberately loads svelte config and passes it on to vite-plugin-svelte so that it has full control, but did you explore https://github.com/sveltejs/vite-plugin-svelte/blob/main/docs/config.md#config-file-resolving ? Monorepo software should have tools to run scripts in various directories, so again i am not sure why a cwd option is needed for sveltekits vite plugin - which would have to be added to vite.config. Example: Line 20 in 657e52a
|
You can check this repo: https://github.com/meeroslav/sveltauri/tree/sveltekit-issue To reproduce:
The |
The
Additionally, adding:
doesn't seem to be having any impact. |
i am not sure how this is supposed to work with users installing dependencies that are not shared between apps or use tooling like If you change all that, i'd say it is your responsibility to also ensure that your script runner set the right cwd beforehand to avoid assumptions inside sveltekit or other tooling from breaking. Happy to discuss how your case can be accomodated, but a PR thread is the wrong place for that. Please open a feature request describing how excactly vite is invoked and what you want to achieve. are you setting vite root differently, how does it work with other frameworks etc. |
the filter command i posted above would work, if there was a package.json with a name in the subpackage...
yes, in sveltekit configFile is currently ignored but it could have been an alternative option to your cwd proposal here... |
@@ -519,7 +517,7 @@ async function kit({ svelte_config }) { | |||
if (vite_config.build.ssr) return; | |||
|
|||
const guard = module_guard(this, { | |||
cwd: vite.normalizePath(process.cwd()), | |||
cwd: vite.normalizePath(cwd), |
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.
should we be introducing a new option for this or using the one that already exists?
cwd: vite.normalizePath(cwd), | |
cwd: vite.normalizePath(vite_config.root), |
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'd much prefer using vite.config.root
(if set) as you suggest.
@meeroslav @dominikg has a feature request been created to add better support for Nx/monorepo projects? |
@dominikg @meeroslav I've just added a Feature Request for this issue to be resolved |
When using SvelteKit in a monorepo, currently you are forced to keep it in the root because the
sveltekit
looks for thesvelte.config.js
in theprocess.cwd()
.This prevents us from:
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.Edits