-
Notifications
You must be signed in to change notification settings - Fork 224
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: allow overriding usage of npx #2848
base: next
Are you sure you want to change the base?
Conversation
I don't love this solution, but throwing it out there for discussion. *Problem* When `workspaces` are in play, `npx` will execute the command with the current working directory set to the `workspace` directory. ``` /root package.json packages my-evidence-proj <- CWD of the vite command ran by npx (incorrect) package.json pages evidence.plugins.yaml ... .evidence template <- CWD of the npx command (correct) tailwind.config.cjs ``` - https://github.com/npm/cli/blob/latest/lib/npm.js#L233-L248 - https://github.com/npm/cli/blob/latest/lib/commands/exec.js#L34-L38 - https://github.com/npm/cli/blob/latest/lib/commands/exec.js#L103 Which ultimately means that `vite` gets run in the wrong `cwd` and thus can't find the template / config. I think arguably this is surprising behavior from `npm` / `npx`, but changing it would probably be a hard sell. Perhaps adding a flag to specify the `cwd` might be accepted? Aside from this, personally I find the use of `npx` for this usecase to be a bit surprising - it's not clear to me how the dependency versions are being controlled. *Possible Solutions* - Change `npm` to allow specifying the `cwd` when running `npx` / `npm exec` in a project using workspaces - Install all dependencies in project and run using `yarn vite` instead of `npx` - Explicitly pass the template root directory to `vite` - this almost works, but `svelte-kit` overrides it / ignores it and ends up being unable to find anything - if it's possible to explicitly pass the template root to svelte-kit instead of relying on the `process.cwd()` that would probably be the cleanest way to solve this
@@ -349,12 +363,18 @@ prog | |||
prog | |||
.command('preview') | |||
.describe('preview the production build') | |||
.option('--debug', 'Enables verbose console logs') |
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.
Note: pretty sure the omission of the --debug
option here is a bug, it's referenced in the action
Great writeup and idea here - I like it, It would probably best to include the configuration in the new The configuration zod schema is here, and it can be accessed with the getEvidenceConfig function. See the template's svelte.config.js for an example usage. I'm not sure what the best property to store this under would be - it could just live as Eventually we'll also be moving over to the new CLI which resolves this issue by directly using vite's API rather than creating a child process |
Description
I don't love this solution, but throwing it out there for discussion.
Problem
When
workspaces
are in play,npx
will execute the command with the current working directory set to theworkspace
directory.Which ultimately means that
vite
gets run in the wrongcwd
and thus can't find the template / config.I think arguably this is surprising behavior from
npm
/npx
, but changing it would probably be a hard sell. Perhaps adding a flag to specify thecwd
might be accepted?Aside from this, personally I find the use of
npx
for this usecase to be a bit surprising - it's not clear to me how the dependency versions are being controlled.Possible Solutions
npm
to allow specifying thecwd
when runningnpx
/npm exec
in a project using workspacesyarn vite
instead ofnpx
vite
svelte-kit
overrides it / ignores it and ends up being unable to find anythingprocess.cwd()
that would probably be the cleanest way to solve thisI've seen everything work with the use
yarn
to runvite
option, and these additional dependencies installed (which also solves all the missingpeerDependency
warnings I get from the sample project):Checklist