-
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
[BUG] Preinstall script runs after installing dependencies #2660
Comments
@t1m0thyj thanks for filing this. We have a consolidated issue tracking work/investigating this: npm/statusboard#267 @wraithgar is looking into it. |
@darcyclarke @wraithgar Thanks for investigating this and updating the documentation. Can this issue please be reopened? I see the linked issue was closed, but the |
today in npm 7 the i would suggest opening an rfc where we can start a discussion about potentially adding a new lifecycle script that runs before anything else. it would have to be a net new hook, though, maybe |
By its name, "preinstall" should not have any dependencies available. imo that it runs after they're installed is just as much of a naming blunder as "prepublish" running on "not just before publish". |
you're not wrong, to be honest a lot of our lifecycle scripts are poorly named and don't run exactly when you expect. that's why there's both a the best we can do right now is clearly document when scripts run and what's available to them |
a lot of the confusion here is that i agree it's not very intuitive, and we'd love to fix it so that the adding these more intuitive hooks could potentially be done as a net new feature though, using a separate top level field in the package.json like "events", that would allow us to keep the current behavior of "scripts" while providing a better implementation elsewhere. eventually we deprecate the usage of "scripts" for anything other than |
I'm also noticing that for npm v6, I know how to run a script prior to installation of dependencies, and for npm v7, I don't. I'm not too attached to the specific methodology of using a I've already looked here but couldn't find anything that seemed to help. |
I also think this should be treated as a bug as previous versions of npm and all versions of yarn support this. For example we use |
Agreed on treating this as a bug. Another example is users who use the Azure: https://github.com/gsoft-inc/azure-devops-npm-auth |
We were also using
The change to npm v7 broke this functionality. |
Facing the same issue right now with CodeArtifact |
- Replace all `npm` commands with `yarn` The reason to do this switch is due to two reasons. The first is that `npm install` completes in 1m30s, while `npm install` completes in 50s, on average. The second is explained in the point below. This runtime difference might appear insignificant, but may improve development time over the long run. - Remove `preinstall` script and change `resolutions` packages The `preinstall` script is misleading and does not run before dependencies are installed, and this is acknowledged in npm/cli#2660 . With the switch to `yarn`, the `preinstall` script becomes obsolete as `yarn` will take care of the `resolutions` without needing a script. In addition, the cause of the security issues was misattributed to the wrong dependency in the previous commit. Remove this and add the true culprits.
* Consolidate dev setup with `npm run setup` which will run `presetup` * CI needs to run `presetup` manually and run `npm i --ignore-scripts` so we don't try to run `husky install` in the `prepare` stage See npm/cli#2660
Another legitimate use case is to force people to use a specific package manager.
|
@beratbayram that doesn't seem like something a dependency should be capable of forcing. |
lockdown.js uses some magic so that it gets run automatically whenever somebody runs `npm install`. This is done by registering a preinstall hook with npm. lockdown.js expects this hook to be called before `npm install` installs the packages and in fact before npm has even fetched any information about the packages from the registry. This used to be the case in older versions of npm but the behavior of npm has changed and now the preinstall hook is run later. This is too late for lockdown.js to intercept the fetches from the registry which leaves things in a broken state. Fixing this seems difficult but luckily for our use case we don't need to run `npm install` interactively and don't care whether that works or or not. Instead we can just call lockdown directly instead of calling `npm install`. This allows things to work as expected. See npm/cli#2660 but note that the exact behavior has changed several times between versions.
Per the documentation for npm ci: This isn't explicitly listed in the npm install, but I assume it may still apply. Additionally, under Best Practices, it states that: What are we supposed to do here? I also attempted a workaround by adding an
If I run
So when running |
@ljharb I presume this was directed at my comment? I'm not suggesting that preinstall be restored to the previous behaviour for login workflows. I'm pointing out that since there is no ability to set a config option at the user level to specify a credential helper to be called by npm when it needs an auth token for a registry, that the preinstall mechanism was misused to facilitate registry services with short lived tokens. There appears to be an increasing desire from a security perspective to limit exposure by only having tokens that live for a couple of hours, reducing the risk when one leaks. What I think many would be happy is having some way to make it possible to provide a workflow that can work across multiple private registries. This may mean having the ability to register a helper for each or multiple registries (by pattern) that will handle returning the token needed. The helper may hook into existing enterprise SSO, or 2 Factor auth, but the key is that when a token is needed it triggers creation or reuses a cached one stored securely in a keychain. This also removes the need for tokens to be stored directly in files. Equally important is that npm shouldn't care how the tools works, only that it conforms to specific input/output behaviour. If the tool needs user interaction, approval, etc, it's on the tool to work out how to support, npm need only provide an interface definition that the tool must conform to. I'll happily log a separate issue for this, but I think it's worth looking at this as one use case that was driving the misuse of preinstall die to the absence of an alternative. |
"As intended" by what line of thinking? I am curious where such things are stated, discussed and confirmed as to be changed?
Did those "problems" not exist before the change was introduced as late as version 7? Assuming they did, then the course of action to address such "already existing" problems is not by breaking existing behaviour that a large number of existing code is relying on, and change it in a way to contradict the literal meaning of "pre-install" to mean "post-install-and-before-compilation-scripts-execute", but instead, introduce a new solution (e.g. lifecycle phase) that can be leveraged by those who had a problem to begin with. Alternatively, if anything, provide an option to enable the originally intended behaviour (and possibly as a differently named lifecycle phase). |
Yeah. This is just weird? Completely breaks the principle of least surprise and even worse - as far as I can tell - there's no alternative that npm provides... From the comments it seems |
Bump on making a decision to fix this or implement a I was very surprised to find out this is how the |
so how do i force a user to use pnpm? i need that |
You don’t? people can use any package manager they want. |
Hi @sebfindling , To force users to use pnpm, please have a look at this package: just-pnpm Hope it helps! :) |
Another reason why people need
|
@SourceCipher preinstallOnly scripts would still have output suppressed, to prevent people polluting installers' terminals with ads and whatnot. |
Any potential update on this? |
Any news? |
We use devcontainers and I wanted to prevent dep installations if not done from a container. The idea in preventing deps to be installed outside the container is to avoid |
Current Behavior:
In NPM v7 the preinstall script runs after dependencies are installed, which breaks backwards compatibility with NPM v6.
This may have been on purpose, but I do not see the behavior change listed under breaking changes.
https://blog.npmjs.org/post/626173315965468672/npm-v7-series-beta-release-and-semver-major
Expected Behavior:
The preinstall script should run before dependencies are installed.
Steps To Reproduce:
Clone this minimal sample repo which contains 2 packages:
package1
- This package is designed to fail if installed standalone. It expects the file "temp.txt" to exist that has been generated by a parent package before install.package2
- This package installspackage1
as a dependency, and has a preinstall script that creates the file "temp.txt".Run
cd package2 && npm install
.With NPM v6, the install succeeds because the preinstall script creates "temp.txt" before
package1
is installed.With NPM v7, the install fails because the preinstall script creates "temp.txt" after
package1
is installed.Environment:
The text was updated successfully, but these errors were encountered: