-
Notifications
You must be signed in to change notification settings - Fork 449
[chore] Replace npx with pnpx across the codebase #5329
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
Conversation
🎭 Playwright Test Results⏰ Completed at: 09/26/2025, 07:36:47 AM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
DrJKL
left a comment
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.
👍🏻
|
Added reusable workflow for cached playwright browser installation, its ready for review again now :D |
DrJKL
left a comment
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.
Would you mind splitting this into two PRs, one that just does the pnpm exec / pnpm dlx and another for the Playwright composite action?
I'm very excited to have less duplication across the actions.
| browsers: | ||
| description: 'Browsers to install (chromium, firefox, webkit, or "all")' | ||
| required: false | ||
| default: 'chromium' |
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 would start with just extracting the current behavior from the actions we're using.
We can always add features later, as we need them.
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.
|
Clean, I like it, but most of these should probably be pnpm exec instead of pnpm dlx, if it's a case where we've already done a pnpm install for the dep. |
- Use 'pnpm exec' for dependencies that are installed (playwright, tsx, lint-staged, nx) - Use 'pnpm dlx' for packages that are run once without installation (wrangler, openapi-typescript, only-allow) - Update MCP config to use 'pnpm dlx' instead of 'pnpx -y'
- Resolved merge conflicts by taking main branch changes - Reapplied npx -> pnpm exec conversions for installed deps - Removed .mcp.json as it was deleted in main
@DrJKL Done! already set auto-merge. will merge if everything works fine |
## Summary - Migrated all `npx` commands to `pnpx` to align with the pnpm ecosystem - Updated all occurrences across the codebase for consistency ## Changes - **Package.json scripts**: Updated test:browser, preinstall, and collect-i18n scripts - **GitHub Actions workflows**: Updated all workflow files that use npx (test-ui, i18n, update-manager-types, etc.) - **Documentation**: Updated browser_tests/README.md and docs/extensions/development.md - **Husky pre-commit hook**: Updated lint-staged and tsx commands - **MCP configuration**: Updated .mcp.json to use pnpx ## Test Plan - [ ] CI tests pass - [ ] Local development commands work with pnpx - [ ] GitHub Actions workflows execute successfully 🤖 Generated with Claude Code ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-5329-chore-Replace-npx-with-pnpx-across-the-codebase-2646d73d36508127bd43d14b8364aeb1) by [Unito](https://www.unito.io)
Summary
npxcommands topnpxto align with the pnpm ecosystemChanges
Test Plan
🤖 Generated with Claude Code
┆Issue is synchronized with this Notion page by Unito