Skip to content
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

fix: prevent postinstall script from running when installed as dependency using pnpm #1255

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sternma
Copy link

@sternma sternma commented Nov 4, 2024

The postinstall script runs patch-package even when the package is installed as a dependency, causing installation failures for users who don't have patch-package installed in their projects. This issue is particularly evident when using pnpm, which handles module isolation more strictly.

Problem:
The postinstall script attempts to run patch-package, which isn't available in user projects, causing installation to fail. The workaround is for the user to add patch-package as a top-level dev-dependency in their own project.

Solution:
This fix updates patches/patch.js to check if the script is running inside a node_modules directory. If it is, the script assumes it's being installed as a dependency and skips running patch-package.

Compatibility:
Tested with npm, pnpm, and yarn. The fix works across all package managers.

Testing:

  • Verified that postinstall runs during local development.
  • Confirmed that installation succeeds without errors in a test project.

@sternma
Copy link
Author

sternma commented Nov 8, 2024

Hey @badeball - I read your proposal in cucumber's repo. Would you still consider completing this PR to unblock pnpm users?

@badeball
Copy link
Owner

badeball commented Nov 9, 2024

Would you still consider completing this PR to unblock pnpm users?

This would at the very least have to test the mentioned package managers. Preferably using a different jobs to test latest version of each, using one of the examples.

@badeball
Copy link
Owner

badeball commented Nov 9, 2024

I'm also not a fan of the heuristics implemented here. You're removing the possibility of working inside a node_modules directory. While I don't do this myself, I see no reason for why this library should not allow it.

Edit: Scratch this - as this will exclusively affect developers of the library, ie. me only.

@badeball
Copy link
Owner

I tried using pnpm to install dependencies of one of the examples (esbuild-cjs) and it runs seemingly fine and without errors. What issue have you observed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants