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(npm): Properly handle error from npm-invoked cli #1302

Merged
merged 1 commit into from
Aug 26, 2024

Conversation

harmony7
Copy link
Member

This PR corrects the case when the fastly command exits with a non-zero exit code when called through the npm wrapper.
Before this fix, the script doesn't run to completion, a stack trace leaks, and the real exit code is not available to the caller.

Example error output when called with non-existent --asdf flag:

ERROR: error parsing arguments: unknown long flag '--asdf'.

node:internal/errors:865
  const err = new Error(message);
              ^

Error: Command failed: /Users/komuro/Library/Application Support/fnm/node-versions/v18.19.1/installation/lib/node_modules/@fastly/cli/node_modules/@fastly/cli-darwin-arm64/fastly --asdf
    at checkExecSyncError (node:child_process:890:11)
    at execFileSync (node:child_process:926:15)
    at file:///Users/komuro/Library/Application%20Support/fnm/node-versions/v18.19.1/installation/lib/node_modules/@fastly/cli/fastly.js:22:1 {
  status: 1,
  signal: null,
  output: [ null, null, null ],
  pid: 22519,
  stdout: null,
  stderr: null
}

Node.js v18.19.1

This happens because the exit code is 1. The wrapper invokes fastly using the child_process.execFileSync() function:

If the process times out or has a non-zero exit code, this method will throw an Error that will include the full result of the underlying child_process.spawnSync().

This PR fixes this by:

  • Wrapping the call to execFileSync() with a try / catch.
  • Checking the thrown err object for code. If this is set then it means execFileSync() itself failed (couldn't launch, timeout, etc.), so we rethrow the error.
  • Checking the thrown err object for status. If this is set then it means the child process has set an exit code, so we set the current process's exit code to the value.

New output:

ERROR: error parsing arguments: unknown long flag '--asdf'.

@harmony7 harmony7 requested a review from kpfleming August 23, 2024 05:35
@kpfleming kpfleming merged commit 8dc6473 into main Aug 26, 2024
9 checks passed
@kpfleming kpfleming deleted the kats/cli-handle-error branch August 26, 2024 12:20
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.

2 participants