Honor the declared package manager - #168
Conversation
0ddb32d to
c451cba
Compare
|
Failing test looks unrelated, I think... 😇 |
|
@Kocal any time to have a quick look here by chance? I'd trust you more than myself here as always :) |
|
@Toflar thank you! I'm asking for advice to my colleague (if he has time), it has nothing to do with your PR, and everything to do with my less-than-average knowledge about JS package managers :) |
There was a problem hiding this comment.
I didn't check, but I'm quite sure (or fully wrong), that the code does not really execute the exact package manager mentioned in the packageManager field.
With Corepack (which is now deprecated/removed) or latest versions of pnpm, which both respect the packageManager field, there is a kind of magic that automatically configure the exact package manager (and its exact version) for your project, that you can run in your shell without any issue.
Here, I think that if you have pnpm version X globally installed and pnpm version Y in packageManager field, then your patch will execute pnpm version X instead of the version Y 🤔
But I may be wrong
There was a problem hiding this comment.
🟡 Changes recommended
Modern Yarn declarations currently select an incompatible Yarn Classic installation command, and fallback coverage is lost.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Honors the consuming project’s declared npm, Yarn, or pnpm package manager before existing fallback detection.
Changes:
- Detects
packageManagerfrom the current project. - Validates the selected manager and updates help text.
- Adds isolated CLI coverage for declared npm.
File summaries
| File | Description |
|---|---|
bin/playwright-install |
Adds declared package-manager detection. |
tests/Integration/Installer/PlaywrightInstallCliTest.php |
Adds npm-selection coverage and isolated executables. |
Review details
- Files reviewed: 1/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| mkdir($binDirectory, 0777, true); | ||
| file_put_contents($projectDirectory.'/package.json', json_encode([ | ||
| 'packageManager' => 'npm@11.6.0', | ||
| ], JSON_THROW_ON_ERROR)); |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Prefer the project packageManager declaration before fallback detection. Allow an explicit package-manager executable through --package-manager-bin and use it throughout installation. Document the override and cover declared-manager selection, fallback behavior, and explicit binaries.
156b5d7 to
e23140e
Compare
|
You’re right, this selects the package manager name but doesn’t enforce the declared version. My original issue was that GitHub CI has Yarn Classic installed, so the installer selected Yarn even though our project declares However, I have also introduced support for an explicit executable override now: vendor/bin/playwright-install --package-manager-bin=/path/to/npm --browsersThat keeps the usual installer checks and comfortable auto-detection while also giving users maximum flexibility to define a different binary locally/in CI. Version management explicitly stays with the environment. Better? |
| private function getDeclaredPackageManager(): ?string | ||
| { | ||
| $packageJsonPath = getcwd().'/package.json'; | ||
|
|
||
| if (!is_file($packageJsonPath)) { | ||
| return null; | ||
| } | ||
|
|
||
| $packageJson = json_decode((string) file_get_contents($packageJsonPath), true); | ||
| $packageManager = is_array($packageJson) ? ($packageJson['packageManager'] ?? null) : null; | ||
|
|
||
| if (!is_string($packageManager)) { | ||
| return null; | ||
| } | ||
|
|
||
| $packageManager = explode('@', $packageManager, 2)[0]; | ||
|
|
||
| return in_array($packageManager, ['npm', 'yarn', 'pnpm'], true) ? $packageManager : null; | ||
| } |
There was a problem hiding this comment.
My previous review still applies here... right? 🤔
There was a problem hiding this comment.
Sorry but I don't know what part of it. I explained why the version is ignored and not part of this PR.
There was a problem hiding this comment.
Ah, sorry I didn't see, yeah it's fine
|
By curiosity, I looked at how Vite is able to detect the package manager (to install deps and run the dev-server when creating a project), and they simply use the Given how Vite is ultra popular, we can say that this solution is battle-tested. To me, we can safely rely on it, and remove |
The installer previously preferred an available Yarn or pnpm executable even when the consuming project declared npm through e.g.
"packageManager": "npm@11.6.0"inpackage.json.The installer now honors supported
packageManagerdeclarations for npm, Yarn, and pnpm. When no supported manager is declared, the existing lockfile and executable detection remains in place.