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

[BUG] npm uninstall -g doesn't run the preuninstall script #3042

Closed
Goobles opened this issue Apr 7, 2021 · 8 comments
Closed

[BUG] npm uninstall -g doesn't run the preuninstall script #3042

Goobles opened this issue Apr 7, 2021 · 8 comments
Assignees
Labels
Bug thing that needs fixing Priority 1 high priority issue Release 7.x work is associated with a specific npm 7 release

Comments

@Goobles
Copy link

Goobles commented Apr 7, 2021

Current Behavior:

npm uninstall -g <package> doesn't run the preuninstall script

Also, npm install -g <package> doesn't run the preuninstall script from the previously installed version of the same package

Installing a newer version of a global package does trigger preuninstall in v6

Expected Behavior:

npm uninstall -g <package> should run the preuninstall script

Steps To Reproduce:

Since npm7 removed console output for scripts during installation, so I echo something to a logfile instead.

  1. Create a package with these scripts:
{
  "name": "testpackage",
  "version":"0.0.1",
  "scripts": {
    "preinstall": "echo \"preinstall\" >> /logs/log.txt",
    "preuninstall": "echo \"preuninstall\" >> /logs/log.txt"
  }
}
  1. run npm pack
  2. run npm install -g ./testpackage-0.0.1.tgz
  3. run npm install -g ./testpackage-0.0.1.tgz again (this should have triggered preuninstall for the previous installation)
  4. run npm uninstall -g testpackage
  5. confirm that the text file in /logs/log.txt does in fact not contain any 'preuninstall' lines

Images:
In v7:
Code_kIGgtizrAQ
In v6:
Code_RSlxOsyZwd

Environment:

  • OS: Windows 10
  • Node: 15.14.0
  • npm: 7.7.6
@Goobles Goobles added Bug thing that needs fixing Needs Triage needs review for next steps Release 7.x work is associated with a specific npm 7 release labels Apr 7, 2021
@wraithgar
Copy link
Member

by design, npm does not have any lifecycle events/scripts that run during uninstall. I would agree that they should be there, but we would have to discuss it as part of our RFC process.

@darcyclarke can we add this issue to the next RFC call?

@wraithgar wraithgar added Agenda will be discussed at the Open RFC call Enhancement new feature or improvement Needs Discussion is pending a discussion and removed Bug thing that needs fixing Needs Triage needs review for next steps labels Apr 8, 2021
@darcyclarke
Copy link
Contributor

@wraithgar I wasn't aware that this was a design decision(?) This definitely did work in v6 (was able to replicate below), feels like a legitimate bug but I could be wrong...

v6.14.12

Screen Shot 2021-04-09 at 1 22 04 AM

v7.9.0

Screen Shot 2021-04-09 at 1 29 15 AM

Our docs explicitly point out uninstall as a lifecycle event (ref. https://docs.npmjs.com/cli/v7/using-npm/scripts#examples) that will run... which doesn't seem to be working either; I also can't find any documentation on a decision to remove this in our changelog, code or RFCs - so it may just be that it was an oversight & we need to add those events explicitly back into rebuild.js when removing a dep from arborist...

@darcyclarke darcyclarke added the Priority 1 high priority issue label Apr 9, 2021
@Goobles
Copy link
Author

Goobles commented Apr 9, 2021

Some packages depend on a preuninstall hook: winser documents it to add and remove background processes on install/uninstall.

I personally use it for an in-house cli tool that installs a local rest api that runs in the background at all times. When installing a new version this service needs to be uninstalled as to not cause file lock issues in Windows, and reinstalled so a newer version of this api is spun up. This is now causing issues in npm7.

@wraithgar
Copy link
Member

@wraithgar I wasn't aware that this was a design decision(?) This definitely did work in v6 (was able to replicate below), feels like a legitimate bug but I could be wrong...

Good catch. I was looking at the Lifecyle Operation Order Which doesn't mention uninstall at all. The only place uninstall is mentioned is in that example itself.

I agree this is now an arborist bug.

@wraithgar wraithgar added Bug thing that needs fixing and removed Agenda will be discussed at the Open RFC call Enhancement new feature or improvement Needs Discussion is pending a discussion labels Apr 9, 2021
@darcyclarke darcyclarke assigned nlf and unassigned nlf Apr 26, 2021
@darcyclarke
Copy link
Contributor

darcyclarke commented May 3, 2021

Actions To Be Taken

  • audit documentation to ensure it's accurate for lifecycle events
  • determine when we were firing this event in v6
  • add run-script call for uninstall to match v6 behavior (note: most likely call in lib/uninstall.js before/after reify)

@darcyclarke darcyclarke added this to the OSS - Sprint 33 milestone Jul 12, 2021
@fritzy fritzy self-assigned this Aug 18, 2021
@fritzy
Copy link
Contributor

fritzy commented Aug 19, 2021

I've audited the code and documentation and it appears to be correct.

npm v6 & v7 take a very different approach to removing packages. The arborist package can be told to add or remove packages, and each time recreates an optimal tree. The old and new tree are diff'd and the corrections are made. Removing a package may happen because of a duplicate found or any other number of reasons.

It would be difficult to present developers with clear documentation on all of the scenarios a package could be removed, and we don't currently have a mechanism for giving those scripts context on why the package was removed (duplicate, user action, changed dependency on a sub-package, changed semver spec requirement, etc).

We could limit execution of uninstall scripts to when a root dependency or global is directly removed by a user, but it is unclear whether that would match the expected use cases or be sufficiently useful.

In the future we could rework these lifecycle scripts to be executed with more context.

@darcyclarke darcyclarke removed this from the OSS - Sprint 35 milestone Aug 23, 2021
@fritzy
Copy link
Contributor

fritzy commented Aug 23, 2021

Next Action Items needed:

  • Update Docs to reflect lack of uninstall lifecycle
  • develop RFC for more robust lifecycle events

@websolutions-hamburg
Copy link

Is there any kind of workaround here? I have a similar constellation as @Goobles and need the preuninstall lifecycle script.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing Priority 1 high priority issue Release 7.x work is associated with a specific npm 7 release
Projects
None yet
Development

No branches or pull requests

6 participants