Skip to content
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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

meeroslav
Copy link

@meeroslav meeroslav commented Jun 28, 2024

When using SvelteKit in a monorepo, currently you are forced to keep it in the root because the sveltekit looks for the svelte.config.js in the process.cwd().

This prevents us from:

  • Having multiple sveltekit apps in nested folders
  • Which we would be able to build/serve from the root of the monorepo

Note: Test seems to be failing with unrelated code


Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Edits

  • Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

Copy link

changeset-bot bot commented Jun 28, 2024

🦋 Changeset detected

Latest commit: dec9864

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Minor

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

@Anthony-Jhoiro
Copy link

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.
If needed, I would be happy to help!

@dominikg
Copy link
Member

dominikg commented Jul 4, 2024

I'm not sure this is needed, you can just set the cwd when invoking the scripts instead:

pnpm --dir apps/app1 build (npm and yarn have similar args)

@meeroslav
Copy link
Author

@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.
The problem is if you are using a monorepo tooling such as Nx, Turbo, Lerna, etc. where you typically run command from the root, but then use configuration to specify the cwd overrides. You can do this via vite.config, but sveltekit plugin does not respect that nor can you currently explicitly pass it as a parameter - which this PR is trying to fix.

@dominikg
Copy link
Member

dominikg commented Jul 5, 2024

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:

"build": "pnpm --filter @sveltejs/* -r build",
builds all public packages in their respective directories in this monorepo

@meeroslav
Copy link
Author

You can check this repo: https://github.com/meeroslav/sveltauri/tree/sveltekit-issue

To reproduce:

  • Clone the [email protected]:meeroslav/sveltauri.git
  • Checkout the branch sveltekit-issue
  • Run pnpm install
  • Run pnpm nx build frontend --verbose

You should see this error:
image

The sveltekit() plugin from frontend/vite.config.js still looks for svelte.config.js in the root rather than the folder where the vite config is.

@meeroslav
Copy link
Author

meeroslav commented Jul 5, 2024

The pnpm --dir ... would not work in the following scenario for 2 reasons:

  • That subfolder doesn't need to have package.json (as is the case in the provided repo)
  • The nx command can be run to build multiple projects at the same time that live in different folders so single dir would not suffice

Additionally, adding:

    svelte({
      configFile: 'frontend/svelte.config.js'
    }),

doesn't seem to be having any impact.

@dominikg
Copy link
Member

dominikg commented Jul 5, 2024

That subfolder doesn't need to have package.json (as is the case in the provided repo)

i am not sure how this is supposed to work with users installing dependencies that are not shared between apps or use tooling like svelte-add that might expect a package.json to exist. create-svelte scaffolds sveltkit applications with a vite.config.js,svelte.config.js and package.json in place and my recommendation would be to keep all that for compatibility within the svelte ecosystem.

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.

@dominikg
Copy link
Member

dominikg commented Jul 5, 2024

The pnpm --dir ... would not work in the following scenario for 2 reasons:

  • That subfolder doesn't need to have package.json (as is the case in the provided repo)
  • The nx command can be run to build multiple projects at the same time that live in different folders so single dir would not suffice

the filter command i posted above would work, if there was a package.json with a name in the subpackage...

Additionally, adding:

    svelte({
      configFile: 'frontend/svelte.config.js'
    }),

doesn't seem to be having any impact.

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),
Copy link
Member

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?

Suggested change
cwd: vite.normalizePath(cwd),
cwd: vite.normalizePath(vite_config.root),

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.

@kristianmandrup
Copy link

@meeroslav @dominikg has a feature request been created to add better support for Nx/monorepo projects?

@kristianmandrup
Copy link

@dominikg @meeroslav I've just added a Feature Request for this issue to be resolved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants