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

[RRFC] Run preinstall / postinstall scripts on single package installation #325

Open
karlhorky opened this issue Feb 17, 2021 · 12 comments
Open

Comments

@karlhorky
Copy link

karlhorky commented Feb 17, 2021

Hi there, first of all, thanks for all of the hard work on npm and everything surrounding it! Really cool what you are doing.

Motivation ("The Why")

The behavior of npm to not run preinstall or postinstall scripts specified by the developer in the project on every npm install <pkg name> is confusing and leads to support requests such as npm/cli#1732

Would you consider starting to run these scripts once during installation of a package?

Example

npm install node-fetch # would also run the `postinstall` script

How

Current Behaviour

preinstall and postinstall scripts are not run during npm install <pkg>

Desired Behaviour

the scripts are run

References

My original use case was to be able to also install the corresponding @types/<pkg name> DefinitelyTyped dep (à la #328). See here for my original naïve approach (which ended up failing): https://github.com/jeffijoe/typesync/pull/65/files


Alternatives Considered

Hook Scripts

In the Open RFC Meeting on Feb 17, 2021 @darcyclarke and others mentioned Hook Scripts.

There are two downsides I've run into so far:

  1. they seem to be broken in npm v7 [BUG] Hook Scripts Not Run on npm install cli#2692
  2. in npm v6, a postinstall hook script, for example, runs once for every package that has been installed. So this means that with a node_modules/.hooks/postinstall script, if you run npm install with your package.json containing 20 dependencies, this script will run at least 20 times, and even more if those dependencies specify transitive dependencies...
@darcyclarke darcyclarke added the Agenda will be discussed at the Open RFC call label Feb 17, 2021
@karlhorky
Copy link
Author

karlhorky commented Feb 18, 2021

Thanks for the responses during the Open RFC Meeting on Feb 17, 2021! Interesting to hear a bit of the background.

I added "Hook Scripts" to a new section above called "Alternatives Considered", along with documenting the roadblocks that I ran into there.

Clarifying the current behavior with docs and separating out the use cases for any future features sounds like good next steps, thanks! 👍

cc @isaacs @wraithgar @darcyclarke @ljharb

@karlhorky
Copy link
Author

Thanks again for the responses during the Open RFC Meeting on Feb 24, 2021!

Sorry for not being able to join again! The timing is kind of difficult for me - maybe I'll be able to join at some point.

A few comments:

@isaacs mixing the semantics of existing preinstall and postinstall and this new proposal of "General-purpose tree-install-has-changed script" is probably not good

@ljharb it should run only for the top-level project and not for any project installing a package that has this lifecycle event

I see what you mean, that makes sense. Right, well if new types of scripts are preferable to keep backwards-compatibility and the existing semantics, then I don't see anything speaking against this. Seems good! 👍

@ljharb might be better to think about from a more user-friendly point of view [instead of using the arborist reify terminology]

Right, if there would be bikeshedding on the name (probably far down the line still), I would also suggest avoiding surfacing these internal technical terms, if possible. Simple, commonly-understandable naming will make searching for and remembering these hooks easier.

@karlhorky
Copy link
Author

Thanks also for the responses during the Open RFC Meeting on Feb 24, 2021!

@isaacs we need a "install tree has been mutated" lifecycle script, that would solve this without conflatig what preinstall/postinstall are used for.

@darcyclarke let's backlog writing an actual RFC for that

Right, this feels like a logical extension of the results of the previous meeting, thanks!

use cases

Just as a quick note about this here (I've also added this to the description above), my original use case was to be able to also install the corresponding @types/<pkg name> DefinitelyTyped dep, à la #328 . See here for my original naïve approach (which ended up failing): https://github.com/jeffijoe/typesync/pull/65/files

@CalculonPrime
Copy link

CalculonPrime commented Apr 23, 2021

I'm curious. Was there any polling of npm users done prior to rolling out this new policy of whether they thought suppressing all install script input/output for all modules was a good idea, just to stop a few bad actors from showing ads when their modules get installed?

I'm puzzled by how you can trust someone to run code as a dependency, but not trust them to print responsibly during install. Don't add dependencies you haven't vetted. Then there is no concern.

@Apollon77
Copy link

@wraithgar closed some issues with reference here where it was about "install" script no longer running. WHen I read here - especially the initial post - this isue is mainly on postinstall/preinstall ... or is install considered too?

Our usecase is that we do some "data housekeeping and corrections" and also user-convenience things in install script and it was executed on any install/update so far. So what's considered as the alternative?

Any yes: I would expect more good behaviour by developers then bad

@CalculonPrime
Copy link

CalculonPrime commented Apr 23, 2021

I would be curious of an estimate of the total number of modules that want to write output during the install script and feel that it provides a benefit to the user, vs. the number of modules that "abused" the feature by printing ads. I'd be really surprised if the former wasn't much larger than the latter.

A phenomena I see often in software is that someone proposes a change due to suggestions, but the vast majority of people who will be affected are never asked how they feel about it, perhaps because they weren't aware of the proposal. (Too much work to monitor design discussions of every tool I use.) Then, having no negative feedback - and why would there be much before most users hit it? - it gets implemented. And then it's a big "political" battle to get it reversed.

@Apollon77
Copy link

Honestly ... there are two things in m eyes: "running the script" and "allowing him to output something if not error stuff". I personally do not care that much about the output, I need the "execute logic" part

@CalculonPrime
Copy link

Yes, well, my issue thread was linked here and closed, so I think this thread is the place for the discussion of not only yours but related issues as well.

@Venryx
Copy link

Venryx commented Jun 7, 2021

Just wanted to add that there's a third problem with the "Hook Scripts" alternative: it doesn't work on Windows. (Windows doesn't recognize the extensionless files as executables)

@darcyclarke
Copy link
Contributor

@Venryx that seems like a bug, can you help file a ticket for that in the cli repo?

Venryx added a commit to Venryx/web-vcore that referenced this issue Aug 6, 2021
…ages without needing to fork them; worth having [especially for installs in docker], even given annoyance here: npm/rfcs#325)

* Added patch that may allow dm_server to start, in docker.
@philippefutureboy
Copy link

Hey there! I just want to jump in to provide a concrete use-case for this.
On my end I'd like to reproduce the dependency groups approach used by poetry in pyproject.toml (python ecosystem).
The rationale is that I'd like to be able to install only the dependencies that I need for each stage of my docker images and for my CI / CD environments.

The approach I would have taken would have been to add pre/post install/uninstall hooks which would automatically update the dependencies.group.{group-name} properties of the package.json through a simple inquirer.js prompt, and another script npm run install:group {group-name} which would cherry pick the dependencies to install in a given environment based on the content of the dependencies.group.{group-name} property.

Without the pre/post install/uninstall hooks, I cannot achieve this result. The only alternative I have considered is to create a subshell for the directory, akin to poetry shell, and add an alias for npm install, npm i that point to shell scripts running the pre and post install scripts, but to me that seems like a surefire way to create more confusion for the developers than otherwise.

So there you go!

@karlismelderis-mckinsey
Copy link

karlismelderis-mckinsey commented Sep 13, 2024

we just disabled postinstall scripts in our pnpm i and it saved us 1-2 minutes in build time for every image and we build 10+ images and that happens multiple times a day so savings are significant

if you ever consider to run postinstall scripts please make them as opt-in per package (including sub-sub-dependencies) in final project configuration

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

No branches or pull requests

7 participants