Repository navigation
Conversation
Author
|
@taylorotwell do u have any suggestion for me to modify/add here to address the issue, so this PR can be mergeable? |
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.
Summary
Closes #15.
pnpm installfails when installing packages with unapproved build scripts, exiting non-zero:[ERR_PNPM_IGNORED_BUILDS] Ignored build scripts: [email protected].Root cause: Please look into the issue #15 for detailed explanation. In short,
Laravel\Chisel\Node\Npm::install()runs the command fromPackageManager::installCommand(), which for pnpm is bare (no--ignore-scripts): PackageManager.php#L28.laravel/installeravoids this failure by passing the--ignore-scriptsflag explicitly inNodePackageManager::installCommand(), which chisel doesn't. so anything driven through Chisel hits it. and this issue applies topnpm11+ only.Why this issue has been silent until now
This is not any regression. it has been latent since pnpm 11, because in practice nothing ever reached Chisel's
pnpmbranch. Let me explain.. Chisel resolves the package manager from disk: a non-npm lock file, otherwise the manager token in thedev/dev:ssr/setupComposer scripts, otherwisenpm. When a starter kit'sinstall:featureshook runs duringlaravel new, the app has just been created. it then carries the kit's committedpackage-lock.jsonand acomposer.jsonstill sayingnpm, because the installer didn't rewrite those scripts until after the hooks had already run (explained inlaravel/installerissue #561).So Chisel always resolved to the default package manager
npm, no matter what--pnpm/--bun/--yarnthe user had passed.npmhas no "build-approval gate", so the bare command (npm install) always exited 0 in that case..I reached this issue while fixing that ordering bug (issue #561) in the
laravel/installer(opened PR #563). So, when I applied the fix, oncecomposer.jsonreflects the correct selected manager before the hooks run, Chisel correctly resolves topnpm, and then this failure surfaces on the very first install.So, the
laravel/installerfix didn't cause the failure; rather it's simply the first thing that ever routes a real install through this code path, which was routed wrongly to the default package manager until now. This issue is equally reachable even without that fix: runphp artisan install:featuresmanually in any project that already has apnpm-lock.yaml.Changes I made
This PR just passes the
--ignore-scriptsflag to thepnpm installcommand. this-laravel/installer(src/Enums/NodePackageManager.php) already does.pnpmversions.laravel/react-starter-kit: lint, format, wayfinder generation, and build all passed with the flag.I left the other commands (other than pnpm install) untouched for now..
Updated test dataset to cover the newly added flag for
pnpm installcommand.