npm / rfcs

Public change requests/proposals & ideation
Other
727 stars 238 forks source link

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

Open karlhorky opened 3 years ago

karlhorky commented 3 years ago

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 https://github.com/npm/cli/issues/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 https://github.com/npm/cli/issues/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...
karlhorky commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

philippefutureboy commented 5 months ago

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 commented 1 week ago

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