Skip to content

perf(browsers): replace execSync with async version detection#493

Merged
mherod merged 4 commits intomainfrom
perf/async-browser-version-488
Mar 15, 2026
Merged

perf(browsers): replace execSync with async version detection#493
mherod merged 4 commits intomainfrom
perf/async-browser-version-488

Conversation

@mherod
Copy link
Copy Markdown
Owner

@mherod mherod commented Mar 15, 2026

Summary

  • Remove execSync from BrowserAvailability.ts entirely
  • Add getBrowserVersionAsync using the existing execSimple utility (async)
  • Remove version detection from detectAvailableBrowsers — version is now lazy/on-demand
  • Update getBrowserInfo to async, calling getBrowserVersionAsync

Eliminates 200-1000ms of event loop blocking from synchronous child processes.

Closes #488

Test plan

  • Type-check passes
  • Biome lint passes
  • All 588 tests pass
  • Pre-commit and pre-push hooks pass

Remove execSync from BrowserAvailability. Version detection is
now async via execSimple and no longer runs during
detectAvailableBrowsers (lazy). getBrowserInfo uses the new
getBrowserVersionAsync for on-demand version fetching.

Fixes #488
@claude
Copy link
Copy Markdown

claude bot commented Mar 15, 2026

PR Review Update: perf(browsers): replace execSync with async version detection

Overview

The previous review raised two concerns: (1) stale JSDoc on detectAvailableBrowsers and (2) getBrowserInfo becoming async as a breaking API change. Revisiting the current PR state:

  • JSDoc on detectAvailableBrowsers — updated and accurate, no longer contains the stale async claim.
  • getBrowserInfo async concern — this was a false positive. getBrowserInfo remains synchronous in this PR. The dismissal of the prior review was correct.

Current Status

The core change is correct and achieves its performance goal. Remaining minor observations:

🔵 Suggestions

  • 2>/dev/null in command strings (BrowserAvailability.ts:301–338): These shell redirects are now vestigial — execSimple captures stderr separately and errors are caught in the outer try/catch. Harmless but slightly misleading.
  • AvailableBrowser.version field: detectAvailableBrowsers and getBrowserInfo no longer populate version. The JSDoc on getBrowserInfo documents this, but the interface field has no deprecation notice. Consumers relying on it will silently get undefined — worth a note in the changelog.
  • Test coverage: getBrowserVersionAsync and getVersionCommand have no dedicated tests. All 588 existing tests pass, and errors degrade gracefully, so this is not blocking — but the new async surface would benefit from at least a couple of unit tests.

👍 What's Good

  • Removing execSync eliminates 200–1000ms of blocking event-loop time during browser detection — a meaningful improvement.
  • getVersionCommand extraction makes the platform-branching logic independently testable.
  • Error handling in getBrowserVersionAsync degrades gracefully with debug-level logging.
  • exactOptionalPropertyTypes is handled correctly: getBrowserInfo omits the version key entirely rather than setting it to undefined.

Summary

LGTM with minor suggestions. No blocking issues — safe to merge.

claude[bot]
claude bot previously requested changes Mar 15, 2026
Copy link
Copy Markdown

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two issues to address before merge: (1) getBrowserInfo becoming async is a public API breaking change - it is re-exported via index.ts and external library consumers will now receive a Promise instead of a value. This needs a semver bump and changelog entry. (2) The JSDoc on detectAvailableBrowsers contains a stale sentence claiming no async/await patterns are used anywhere in the codebase. The core async refactor is correct and the graceful error handling is good.

Address review feedback on PR #493:
- Revert getBrowserInfo to sync (no breaking API change)
- Remove version from getBrowserInfo (use getBrowserVersionAsync)
- Update detectAvailableBrowsers JSDoc to reflect async split
@mherod mherod dismissed claude[bot]’s stale review March 15, 2026 17:56

Both issues addressed: getBrowserInfo kept sync (no breaking change), stale JSDoc updated.

Copy link
Copy Markdown

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: perf(browsers): replace execSync with async version detection

Both issues from the previous (dismissed) review have been addressed:

  • getBrowserInfo API — reverted to synchronous, no breaking change.
  • Stale JSDocdetectAvailableBrowsers JSDoc now correctly documents the sync/async split.

Code quality

  • getVersionCommand extraction is clean and makes the platform logic independently testable.
  • getBrowserVersionAsync error handling degrades gracefully with debug-level logging.
  • exactOptionalPropertyTypes handled correctly — getBrowserInfo returns an inline object with no version key rather than setting it to undefined.
  • Removing execSync eliminates 200–1000ms of blocking from browser detection — a meaningful improvement.

Minor (non-blocking)

  • 2>/dev/null in command strings is now redundant since execSimple captures stderr and errors are caught in the outer try/catch. Harmless but slightly misleading.
  • getBrowserVersionAsync and getVersionCommand have no dedicated unit tests. Existing 588 tests still pass and errors degrade gracefully, so not blocking.
  • The AvailableBrowser.version field is now silently undefined for consumers who called detectAvailableBrowsers or getBrowserInfo expecting it — worth a changelog note if downstream consumers exist.

Verdict

Approved. No blocking issues; the core change is correct and the review feedback was fully addressed.

@mherod mherod merged commit fe76231 into main Mar 15, 2026
15 checks passed
@mherod mherod deleted the perf/async-browser-version-488 branch March 15, 2026 20:49
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.

perf(browsers): replace execSync with async version detection in BrowserAvailability

1 participant