-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Fix Ctrl+C handling on Windows #5758
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
base: dev
Are you sure you want to change the base?
Conversation
| name: "yarn" as const, | ||
| command: () => $`yarn global list`.throws(false).text(), | ||
| }, | ||
| { | ||
| name: "pnpm" as const, | ||
| command: () => $`pnpm list -g --depth=0`.throws(false).text(), | ||
| }, | ||
| { | ||
| name: "bun" as const, | ||
| command: () => $`bun pm ls -g`.throws(false).text(), | ||
| }, | ||
| { | ||
| name: "brew" as const, | ||
| command: () => $`brew list --formula opencode`.throws(false).text(), |
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.
woudlnt u need to do this for all these?
Hmmm and we use "$" elsewhere too would this affect everything?
Are we sure there isn't just a arg we need to send to $ to ensure it doesnt "take over" like it was?
Also did u see open bun discussions?
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.
I tried all args and methods of Bun.$ and nothing worked.
I tested now also other commands for yarn, pnpm and bun and both yarn and pnpm triggered the same bug, even though they don't get executed on my system without manual modification of the code. I'm going to modify the pull request to include those as well.
For the other Bun.$ usages, I see that it quite used in the codebase but I'm not sure if all of those usages are affected. The other usages $ with npm, pnpm and yarn seem to be the ones affected so it might be necessary to add the wrapper there as well.
Potentially other commands might be affected but I haven't found them.
Which bun discussion are you referring to? I only saw the latest comment on #5489, but I don’t really have enough informations about Bun’s internals to tell whether it’s the same issue or not. This one seems specifically related to Bun.$, but I’m not sure if that’s the root cause or if there’s a better workaround.
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.
that makes sense ig I was just asking if u saw a discussion on bun like if u know if anyoen else has seen this issue basically
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.
Oh got it, I’ve seen other issues about Bun not handling Ctrl+C correctly on Windows but in other conditions, nothing specifically related to Bun.$
|
I changed all occurrences of npm, pnpm and yarn to use spawnWrapper, excluding the ones that don't get used during OpenCode execution (for example as a standalone script like the publish script) |
|
IT seems like the bug actually is in Bun |
Closes #5071
To fix the bug described in the issue, I introduced a small wrapper around
Bun.spawnthat kind of mimics theBun.$interface for the usage patterns (.text()and.throw(false)) related to the two problematic commands (npm listandnpm config get registry).I replaced the
$calls with this wrapper, and as a result the Ctrl+C handling now behaves correctly on Windows: it exits if no input prompt is present, otherwise it deletes the current input.I placed the wrapper file in the same directory as the affected file to keep it close, since the bug currently impacts only that file. Additionally, the issue may be fixed upstream in Bun, in which case this wrapper would no longer be necessary.