-
-
Notifications
You must be signed in to change notification settings - Fork 10
refactor: replace Bun APIs with Node.js equivalents #984
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,8 +14,10 @@ | |
| * so that subsequent bare `sentry cli upgrade` calls use the same channel. | ||
| */ | ||
|
|
||
| import { spawn } from "node:child_process"; | ||
|
Check failure on line 17 in src/commands/cli/upgrade.ts
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Node.js spawn errors swallowed in Promise, making EBUSY retry and ENOENT detection dead code Unlike Evidence
Also found at 1 additional location
Identified by Warden find-bugs · HM4-JQ6 |
||
| import { homedir } from "node:os"; | ||
| import { dirname } from "node:path"; | ||
| import { setTimeout } from "node:timers/promises"; | ||
| import type { SentryContext } from "../../context.js"; | ||
| import { | ||
| determineInstallDir, | ||
|
|
@@ -425,12 +427,14 @@ | |
| ): Promise<number> { | ||
| for (let attempt = 1; attempt <= SPAWN_MAX_ATTEMPTS; attempt++) { | ||
| try { | ||
| const proc = Bun.spawn([binaryPath, ...args], { | ||
| stdout: "inherit", | ||
| stderr: "inherit", | ||
| const proc = spawn(binaryPath, args, { | ||
| stdio: ["ignore", "inherit", "inherit"], | ||
| env, | ||
| }); | ||
| return await proc.exited; | ||
| return await new Promise<number>((resolve) => { | ||
| proc.on("close", (code) => resolve(code ?? 1)); | ||
| proc.on("error", () => resolve(1)); | ||
| }); | ||
|
Check failure on line 437 in src/commands/cli/upgrade.ts
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Spawn errors silently swallowed, breaking retry logicHigh Severity In Node.js, Reviewed by Cursor Bugbot for commit 98702ba. Configure here. |
||
| } catch (error) { | ||
| // Translate the opaque Bun "Executable not found" error into an | ||
| // actionable UpgradeError. This path triggers when the binary at | ||
|
|
@@ -454,7 +458,7 @@ | |
| log.warn( | ||
| `Binary is locked (antivirus scan?), retrying in ${delay}ms... (attempt ${attempt}/${SPAWN_MAX_ATTEMPTS})` | ||
| ); | ||
| await Bun.sleep(delay); | ||
| await setTimeout(delay); | ||
| } | ||
| } | ||
| // Unreachable — the loop either returns or throws | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,10 +2,18 @@ | |
| * Browser utilities | ||
| * | ||
| * Cross-platform utilities for interacting with the user's browser. | ||
| * Uses Bun.spawn and Bun.which for process management. | ||
| * Uses child_process.spawn and whichSync for process management. | ||
| */ | ||
|
|
||
| import { spawn } from "node:child_process"; | ||
| import { setTimeout } from "node:timers/promises"; | ||
| import { generateQRCode } from "./qrcode.js"; | ||
| import { whichSync } from "./which.js"; | ||
|
|
||
| /** No-op error handler to prevent unhandled error crashes from spawn. */ | ||
| function noop(): void { | ||
| // Intentionally empty — absorbs async spawn errors (e.g., ENOENT) | ||
| } | ||
|
|
||
| /** | ||
| * Open a URL in the user's default browser. | ||
|
|
@@ -20,10 +28,10 @@ | |
| let args: string[]; | ||
|
|
||
| if (platform === "darwin") { | ||
| command = Bun.which("open"); | ||
| command = whichSync("open"); | ||
| args = [url]; | ||
| } else if (platform === "win32") { | ||
| command = Bun.which("cmd"); | ||
| command = whichSync("cmd"); | ||
| args = ["/c", "start", "", url]; | ||
| } else { | ||
| // Linux and other Unix-like systems - try multiple openers | ||
|
|
@@ -35,7 +43,7 @@ | |
| "kde-open", | ||
| ]; | ||
| for (const opener of linuxOpeners) { | ||
| command = Bun.which(opener); | ||
| command = whichSync(opener); | ||
| if (command) { | ||
| break; | ||
| } | ||
|
|
@@ -48,16 +56,18 @@ | |
| } | ||
|
|
||
| try { | ||
| const proc = Bun.spawn([command, ...args], { | ||
| stdout: "ignore", | ||
| stderr: "ignore", | ||
| const proc = spawn(command, args, { | ||
| stdio: ["ignore", "ignore", "ignore"], | ||
| }); | ||
| // Prevent unhandled error crash if the binary disappears between | ||
| // whichSync() and spawn() (TOCTOU window). | ||
| proc.on("error", noop); | ||
|
|
||
| // Give browser time to open, then detach | ||
| await Bun.sleep(500); | ||
| await setTimeout(500); | ||
|
cursor[bot] marked this conversation as resolved.
|
||
| proc.unref(); | ||
| return true; | ||
| } catch { | ||
|
Check warning on line 70 in src/lib/browser.ts
|
||
|
sentry-warden[bot] marked this conversation as resolved.
Comment on lines
+64
to
70
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Spawn errors silently absorbed by When Evidence
Identified by Warden find-bugs · HK4-FVB |
||
| return false; | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,7 @@ | |
|
|
||
| // biome-ignore lint/performance/noNamespaceImport: Sentry SDK recommends namespace import | ||
| import * as Sentry from "@sentry/node-core/light"; | ||
| import { compare as semverCompare } from "semver"; | ||
|
|
||
| import { | ||
| GITHUB_RELEASES_URL, | ||
|
|
@@ -465,15 +466,15 @@ | |
| const version = tag.slice(PATCH_TAG_PREFIX.length); | ||
| // Include tags where: currentVersion < version <= targetVersion | ||
| if ( | ||
| Bun.semver.order(version, currentVersion) === 1 && | ||
| Bun.semver.order(version, targetVersion) !== 1 | ||
| semverCompare(version, currentVersion) === 1 && | ||
| semverCompare(version, targetVersion) !== 1 | ||
| ) { | ||
| chainTags.push({ tag, version }); | ||
| } | ||
| } | ||
|
|
||
| // Sort by version (chronological for nightlies) | ||
| chainTags.sort((a, b) => Bun.semver.order(a.version, b.version)); | ||
| chainTags.sort((a, b) => semverCompare(a.version, b.version)); | ||
|
Check warning on line 477 in src/lib/delta-upgrade.ts
|
||
|
Comment on lines
+469
to
+477
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Replace Evidence
Identified by Warden find-bugs · ARU-GUD |
||
|
|
||
| return chainTags.map((t) => t.tag); | ||
| } | ||
|
|
||


Uh oh!
There was an error while loading. Please reload this page.