feat: add check command
#59
Conversation
commit: |
|
|
||
| // avoids printing the stack trace for `sv` when `svelte-check` exits with an error code | ||
| try { | ||
| execSync(`npx svelte-check ${args.join(' ')}`, { stdio: 'inherit', cwd }); |
There was a problem hiding this comment.
should we try to use the user's package manager here? while generally unlikely, there's a chance they haven't installed npm and don't have npx. maybe it could happen on a CI where they don't want to install multiple package managers or something
There was a problem hiding this comment.
yea, i tried doing that butpackage-manager-detector's COMMANDS map doesn't include the commands for executing local packages (e.g. pnpm -> pnpm exec)
There was a problem hiding this comment.
it's already installed when you get here at which point npm, pnpm, etc. should work just the same as npx?
There was a problem hiding this comment.
it works with npx because it's used for both executing local and remote pacakges. however, if we were to use the COMMANDS['execute'] with pnpm, it'll resolve to pnpm dlx svelte-check, which will execute the remote package instead of the local one
There was a problem hiding this comment.
and i dont know if the case is similar with the other PMs as well (bun, yarn, etc)
There was a problem hiding this comment.
i guess the problem is that COMMANDS['execute'] should really be renamed to COMMANDS['remote'] or COMMANDS['dlx'] to signify that it's only for executing remote packages. then we could reliably use COMMANDS['execute'] for executing local packages
see: https://github.com/antfu-collective/package-manager-detector/blob/main/src/commands.ts
There was a problem hiding this comment.
raised a PR for this change here: antfu-collective/package-manager-detector#15
There was a problem hiding this comment.
lgtm. I only had one question
the other thing I was wondering for a minute was if we should add svelte-check as an optional peer dependency, but I'm not really sure if there's a benefit to doing so and it looks like npm keeps going back and forth on whether they should be installed by default. I think omitting as done in this PR might be better because we won't install it and the user can if they need to
|
Don't really want to hold off on merging this PR because of antfu-collective/package-manager-detector#15, so I've raised #61 so that we can address it once it does merge |
closes #39
run with: