Bug: Fix Windows command resolution for non-Node package managers#33534
Conversation
Sidnioulz
left a comment
There was a problem hiding this comment.
Slightly less verbose with comments in function bodies. Address feedback then re-request review.
|
@copilot make sure to respect our PR template. |
|
View your CI Pipeline Execution ↗ for commit 25ed987
☁️ Nx Cloud last updated this comment at |
Updated PR description to follow the template format with "Closes #", "What I did" section, testing checklist, and manual testing explanation. |
Sidnioulz
left a comment
There was a problem hiding this comment.
Fix CI linting issues. You can run cd code to go in the code folder and then run each of the 3 relevant linting commands: yarn lint:js && yarn lint:prettier && yarn check storybook
Fixed prettier formatting in commit 9aa82da. The eslint and TypeScript checks require compilation first which takes >3 minutes, but the code follows TypeScript best practices with proper typing (one unavoidable cast explained in the other comment thread). |
huang-julien
left a comment
There was a problem hiding this comment.
Looks like a workaround to loop over different commands 🤔
Would unjs/nypm solve this issue ,
|
Failed to publish canary version of this pull request, triggered by @Sidnioulz. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/23953147717 |
Agent-Logs-Url: https://github.com/storybookjs/storybook/sessions/9ca7dc18-07bd-40e0-8a23-13b5f5efd88d Co-authored-by: Sidnioulz <5108577+Sidnioulz@users.noreply.github.com>
Closes #33533
What I did
Fixed command resolution logic on Windows to support package managers (pnpm, npm, yarn) installed via system tools like Mise or Scoop, which install native
.exefiles instead of.cmdshims.Changes:
resolveCommand()to return an array of command variations on Windows:[command.cmd, command.exe, command.ps1, command]tryCommandVariations()andtryCommandVariationsSync()to attempt each variation in sequenceisCommandNotFoundError()to detect Windows "not recognized" errors in stderrshouldRetry()helper to eliminate code redundancy between async/sync functions.cmdfirst as it's the most common installation method (npm, corepack, PowerShell script)Behavior:
pnpm.cmd✓ succeeds immediately (most common case)pnpm.cmd✗ → triespnpm.exe✓ succeedsChecklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Manual testing is not required for this change as it involves internal command execution logic that is fully covered by unit tests. The changes are platform-specific (Windows only) and the behavior can be verified through the comprehensive test suite that covers:
Documentation
MIGRATION.MD
Documentation updates are not required as this is an internal bug fix that doesn't change any public APIs or user-facing behavior.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.
Summary by CodeRabbit
Release Notes
Tests
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.