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

fix: reject with error from parent context on close #102

Merged
merged 3 commits into from
Apr 15, 2024

Conversation

lukekarrys
Copy link
Contributor

@lukekarrys lukekarrys commented Apr 14, 2024

This PR does two things:

  • When the spawned process is closed with a code or signal, it rejects with an error from the parent context to preserve that more useful stack trace
  • Changes the error message from command failed to @npmcli/promise-spawn command failed. I feel like command failed looks too generic when thrown in the context of the npm CLI and this makes it clear that the error came from spawning something. I removed this from the PR since it could be a breaking change since npm looks for this error message. It would be better to give this a better message further up the stack.

It also refactors a bit to remove one level of nesting. It uses the Promise.withResolvers() pattern to hoist the resolve and reject functions since none of the work actually needs to be done within the Promise executor.

Example

Here's a before and after of npm rebuild --loglevel verbose of a failing preinstall script. Some output is removed for clarity/brevity.

before

npm verb title npm rebuild
npm verb argv "rebuild" "--loglevel" "verbose"
npm info run debug-init-cancel@1.0.0 preinstall  bash ./my-preinstall-script.sh
npm info run debug-init-cancel@1.0.0 preinstall { code: 1, signal: null }
npm verb stack Error: command failed
npm verb stack     at ChildProcess.<anonymous> (/Users/lukekarrys/Desktop/npm-debug/.versions/npm-10.5.2/node_modules/@npmcli/promise-spawn/lib/index.js:53:27)
npm verb stack     at ChildProcess.emit (node:events:518:28)
npm verb stack     at maybeClose (node:internal/child_process:1105:16)
npm verb stack     at Socket.<anonymous> (node:internal/child_process:457:11)
npm verb stack     at Socket.emit (node:events:518:28)
npm verb stack     at Pipe.<anonymous> (node:net:337:12)
npm verb pkgid debug-init-cancel@1.0.0
npm ERR! code 1
npm ERR! path /Users/lukekarrys/Desktop/npm-debug/debug-init-cancel
npm ERR! command failed
npm ERR! command sh -c bash ./my-preinstall-script.sh
npm ERR! Somes script output
npm ERR! Oh no, this will error!
npm verb exit 1
npm verb code 1

after

npm verb title npm rebuild
npm verb argv "rebuild" "--loglevel" "verbose"
npm info run debug-init-cancel@1.0.0 preinstall  bash ./my-preinstall-script.sh
npm info run debug-init-cancel@1.0.0 preinstall { code: 1, signal: null }
npm verb stack Error: command failed
npm verb stack     at promiseSpawn (/Users/lukekarrys/projects/npm/cli/node_modules/@npmcli/promise-spawn/lib/index.js:22:22)
npm verb stack     at spawnWithShell (/Users/lukekarrys/projects/npm/cli/node_modules/@npmcli/promise-spawn/lib/index.js:124:10)
npm verb stack     at promiseSpawn (/Users/lukekarrys/projects/npm/cli/node_modules/@npmcli/promise-spawn/lib/index.js:12:12)
npm verb stack     at runScriptPkg (/Users/lukekarrys/projects/npm/cli/node_modules/@npmcli/run-script/lib/run-script-pkg.js:75:13)
npm verb stack     at runScript (/Users/lukekarrys/projects/npm/cli/node_modules/@npmcli/run-script/lib/run-script.js:9:12)
npm verb stack     at /Users/lukekarrys/projects/npm/cli/workspaces/arborist/lib/arborist/rebuild.js:333:17
npm verb stack     at run (/Users/lukekarrys/projects/npm/cli/node_modules/promise-call-limit/dist/commonjs/index.js:67:22)
npm verb stack     at /Users/lukekarrys/projects/npm/cli/node_modules/promise-call-limit/dist/commonjs/index.js:84:9
npm verb stack     at new Promise (<anonymous>)
npm verb stack     at callLimit (/Users/lukekarrys/projects/npm/cli/node_modules/promise-call-limit/dist/commonjs/index.js:35:69)
npm verb pkgid debug-init-cancel@1.0.0
npm ERR! code 1
npm ERR! path /Users/lukekarrys/Desktop/npm-debug/debug-init-cancel
npm ERR! command failed
npm ERR! command sh -c bash ./my-preinstall-script.sh
npm ERR! Some script output
npm ERR! Oh no, this will error!
npm verb exit 1
npm verb code 1

@lukekarrys lukekarrys requested a review from a team as a code owner April 14, 2024 21:15
lib/index.js Dismissed Show dismissed Hide dismissed
@lukekarrys lukekarrys changed the title fix: create close error in root function for better stack trace fix: reject with error from parent context on close Apr 14, 2024
@wraithgar
Copy link
Member

Everything looks good here, I get why we're doing it, but it still makes me nervous in light of things like npm/run-script#188. Let's be extra wary of new issues we didn't expect.

@lukekarrys lukekarrys merged commit 4912015 into main Apr 15, 2024
20 checks passed
@lukekarrys lukekarrys deleted the lk/close-stack-trace branch April 15, 2024 02:42
@github-actions github-actions bot mentioned this pull request Apr 15, 2024
@github-actions github-actions bot mentioned this pull request May 4, 2024
lukekarrys pushed a commit that referenced this pull request May 4, 2024
🤖 I have created a release *beep* *boop*
---


## [7.0.2](v7.0.1...v7.0.2)
(2024-05-04)

### Bug Fixes

*
[`4912015`](4912015)
[#102](#102) reject with error
from parent context on close (#102) (@lukekarrys)

### Chores

*
[`09872d7`](09872d7)
[#105](#105) linting:
no-unused-vars (@lukekarrys)
*
[`70f0eb7`](70f0eb7)
[#105](#105) bump
@npmcli/template-oss to 4.22.0 (@lukekarrys)
*
[`82ae2a7`](82ae2a7)
[#105](#105) postinstall for
dependabot template-oss PR (@lukekarrys)
*
[`2855879`](2855879)
[#104](#104) bump
@npmcli/template-oss from 4.21.3 to 4.21.4 (@dependabot[bot])

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

None yet

2 participants