-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Use Installer when packaging git dependencies #189
Conversation
This fixes a couple of config issues we've found when installing packages in `dry-run` mode to only get the lockfile changes. When setting `dry-run` or `ignore-scripts`, the setting isn't passed through when installing git dependencies meaning the `prepare` script is always run. We've also found an issue with proxying existing npm config to the cli when a large `cafile` has been set, as this is expanded into the `ca` config key with the full string contents of the ca file. If the ca file is huge this will blow up on linux with an E2BIG exception (Argument list too long). Increasing the limit requires kernel recompilation so isn't really an option. The current implementation requires `prepublish` to be fully deprecated, is this something you are planning? Otherwise I'm happy to find another way of passing this config through the installer.
Fully deprecating |
@isaacs great news! Can definitely wait for v7, we've worked around it on Dependabot so don't have an urgent need. |
What’s the point of fully deprecating it, as opposed to just making it be the same as prepublishOnly? |
@isaacs what's the status on deprecating |
Hey @feelepxyz! Sorry for the delay, we're going to close this PR as we've completely refactored this code in |
When installing git dependencies, the config options:
dry-run
andignore-scripts
are being lost if set as cli arguments causing theprepare
script to always run. We found this issue while doing an install that only needs to get lockfile changes. You can work around this by setting these settings in a.npmrc
file.We've also found an issue with proxying npm config to the cli when a large
cafile
has been set. The contents ofcafile
is expanded into theca
config key which get passed as a cli argument to install. When this file is big (e.g. including a custom cert + all system certs) it causes a kernel exception on Linux (E2BIG: Argument list too long).The current implementation requires
prepublish
to be fully deprecated because I couldn't figure out a way to only set theignore-prepublish
config setting for the git install. Setting it withnpm.config.set('ignore-prepublish', true)
before the install doesn't seem to be picked up in thenpm-lifecycle
package when running theprepublish
stage.I imagine it will be a pain to deprecated
prepublish
without breaking a bunch of packages, is this still something you're planning or should I find a workaround?