chore: use tinyexec - #1213
Conversation
|
Install the latest version of pnpm add https://pkg.svelte.dev/sv/c/ea6eb5884f9b5ad9f04513d02d7ff871c978a8c0Open in Note This PR is from a fork. A maintainer must approve approve each commit before it can be built and installed. |
|
sacrosanctic
left a comment
There was a problem hiding this comment.
Not sure if these are supposed to throw.
| if (process.platform === 'win32') { | ||
| // on windows, use taskkill to terminate the process tree | ||
| await exec('taskkill', ['/PID', `${pid}`, '/T', '/F']); | ||
| await exec('taskkill', ['/PID', `${pid}`, '/T', '/F'], { throwOnError: true }); |
There was a problem hiding this comment.
i'd wager to say no, it shouldn't throw given that it didn't throw before. im also guessing it exits with a non-zero exit code if the process no longer exists, which we wouldnt want to throw on anyway
| @@ -363,7 +363,10 @@ export function createSetupTest( | |||
| } | |||
|
|
|||
| const installDir = path.resolve(cwd, testName); | |||
| const install = await exec('pnpm', ['install'], { nodeOptions: { cwd: installDir } }); | |||
| const install = await exec('pnpm', ['install'], { | |||
| nodeOptions: { cwd: installDir }, | |||
| throwOnError: true | |||
There was a problem hiding this comment.
this should not throw since it's being handled in the lines directly underneath it
| import fs from 'node:fs'; | ||
| import path from 'node:path'; | ||
| import { execSync } from 'tinyexec'; |
There was a problem hiding this comment.
Should be the same
| if (reasons.length === 0) return { success: true, message: undefined }; | ||
| throw new UnsupportedError(reasons); |
There was a problem hiding this comment.
my old eyes aren't what they used to be - it's easier to read with the spacing :p
| if (reasons.length === 0) return { success: true, message: undefined }; | |
| throw new UnsupportedError(reasons); | |
| if (reasons.length === 0) { | |
| return { success: true, message: undefined }; | |
| } | |
| throw new UnsupportedError(reasons); |
| await exec(command, args, { nodeOptions: { cwd }, throwOnError: true }); | ||
| return {}; | ||
| } catch (e) { | ||
| // @ts-expect-error tinyexec rethrows the spawn error as-is |
There was a problem hiding this comment.
// @ts-expect-error tinyexec rethrows the spawn error as-is
I know you didn't change this catch-block, but is this still correct? Looking through NonZeroExitError, I don't see e.code as an available prop.
|
|
||
| try { | ||
| execSync('docker --version', { cwd, stdio: 'pipe' }); | ||
| execSync('docker', ['--version'], { nodeOptions: { cwd } }); |
There was a problem hiding this comment.
this should throw
| execSync('docker', ['--version'], { nodeOptions: { cwd } }); | |
| execSync('docker', ['--version'], { nodeOptions: { cwd }, throwOnError: true }); |
| if (process.platform === 'win32') { | ||
| // on windows, use taskkill to terminate the process tree | ||
| await exec('taskkill', ['/PID', `${pid}`, '/T', '/F']); | ||
| await exec('taskkill', ['/PID', `${pid}`, '/T', '/F'], { throwOnError: true }); |
There was a problem hiding this comment.
i'd wager to say no, it shouldn't throw given that it didn't throw before. im also guessing it exits with a non-zero exit code if the process no longer exists, which we wouldnt want to throw on anyway
| fs.writeFileSync(pageServerPath, pageServer, 'utf8'); | ||
|
|
||
| execSync('npm run db:push', { cwd, stdio: 'pipe' }); | ||
| execSync('npm', ['run', 'db:push'], { nodeOptions: { cwd } }); |
There was a problem hiding this comment.
execSync from node:child_process throws by default on non-zero exit codes. it should probably throw here as well
| execSync('npm', ['run', 'db:push'], { nodeOptions: { cwd } }); | |
| execSync('npm', ['run', 'db:push'], { nodeOptions: { cwd } }); |
|
|
||
| // Generate auth schema using better-auth CLI | ||
| execSync('npm run auth:schema', { cwd, stdio: 'pipe' }); | ||
| execSync('npm', ['run', 'auth:schema'], { nodeOptions: { cwd } }); |
|
|
||
| // Push schema to DB | ||
| execSync('npm run db:push -- --force', { cwd, stdio: 'pipe' }); | ||
| execSync('npm', ['run', 'db:push', '--', '--force'], { nodeOptions: { cwd } }); |
| } | ||
|
|
||
| if (dockerInstalled) execSync('docker compose up --detach', { cwd, stdio: 'pipe' }); | ||
| if (dockerInstalled) execSync('docker', ['compose', 'up', '--detach'], { nodeOptions: { cwd } }); |
There was a problem hiding this comment.
this one should probably throw (or at least be handled in some way) as well if we're depending on it running successfully
|
|
||
| fs.writeFileSync(path.join(test_workspace_dir, 'pnpm-workspace.yaml'), 'packages:\n - ./*\n'); | ||
|
|
||
| const exec_async = promisify(nodeExec); |
There was a problem hiding this comment.
hmmm, there was a reason we used this instead due to failing tests, but if it's no longer failing then we're probably good
Co-authored-by: CokaKoala <31664583+AdrianGonz97@users.noreply.github.com>
Closes #
Description
Checklist