Repository navigation
Conversation
On Windows, the target command always runs through a `powershell.exe` launcher, which can take noticeably longer than the ~10-30ms assumed for other platforms' launchers to start executing. The non-fallback path only waited for the `spawn` event before resolving and letting the caller's process exit, so a caller exiting right after `await open(...)` could tear down the still-starting, non-detached launcher before it ran `Start-Process` and actually opened the target app. Reuse the existing fallback-attempt handling, which already waits for the launcher's `close` event, for the Windows path as well. This does not wait for the opened app itself, only for the launcher script. Fixes sindresorhus#298
Owner
|
Closing in favor of 734b821 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
On Windows,
open()(withoutwait: true) can silently fail to open anything, even though it resolves without throwing.This reproduces with a plain call like:
This is not specific to any particular runner (npm scripts,
npx, etc.) — it reproduces with a barenodeinvocation that does nothing after theawait.Root cause
On Windows, the actual command is always wrapped in a
powershell.exelauncher that runs a base64-encodedStart-Process ...script. The comment above the non-fallback resolution path assumes:That assumption holds for macOS
openandxdg-open, but not for legacy Windows PowerShell (System32\WindowsPowerShell\v1.0\powershell.exe, whichpowerShellPath()always targets — Windows still isn't defaulting new processes topwsh, so this is the common case, not an edge case). Starting it up (loading the runtime, applying-ExecutionPolicy Bypass, decoding and parsing the script) routinely takes several hundred ms, well past that ~10-30ms assumption.The non-fallback, non-
waitpath only waits for the'spawn'event before resolving:'spawn'only means the OS handed back a process handle forpowershell.exe— not that it has finished executing the encodedStart-Processcommand yet. If the caller does nothing meaningful afterawait open(...)(a very common pattern, e.g. a short-lived CLI script), the process can exit right after this resolves. Since the Windows branch never setsdetached: trueonchildProcessOptions(only the Linuxxdg-openbranch does), the still-startingpowershell.exechild is not detached from the caller, and can be torn down before it ever gets to runStart-Process.I confirmed this directly: replacing the
spawn-based resolution with one that waits for the launcher's'close'event (what the existingisFallbackAttemptpath already does) makes it succeed reliably, taking ~200-900ms depending on system load — consistent with legacy PowerShell startup cost, not with the app itself.Related: #298 (closed as "probably fixed" by 966239c, which added the
'spawn'wait — that made things better but didn't fully close the race on Windows specifically, for the reason above). Also related: #144, #189.Fix
Reuse the existing
isFallbackAttempthandling (which already waits for the launcher's'close'event before resolving) for the Windows path too, since Windows always goes through this same kind of short-lived launcher process. This does not wait for the opened app to close — only for the (still fast, just not ~10-30ms fast) launcher script thatStart-Processruns in. It's unrelated tooptions.wait, which additionally passes-WaittoStart-Processitself to block until the opened app exits.Testing
xopasses on the changed files.avatest mirroring the existing'app launches resolve before close without fallback'test, but asserting the opposite (resolves after close) — gated byprocess.platform === 'win32', leaving the non-Windows test and behavior untouched.bravelaunch on Windows 11: without this change, the browser did not open; with it,open()waits for the launcher and the browser reliably opens.Fixes #298