search

jzaefferer / commitplease

Validates strings as commit messages
87 stars 17 forks source link

Uninstall: fix automatic commit-msg removal #66

Closed alisianoi closed 8 years ago

alisianoi commented 8 years ago

My nvm --version is 0.31.0

With the following node/npm combinations

  1. node --version at v0.12.15 and npm --version at 2.15.1
  2. node --version at 4.4.7 and npm --version at 2.15.8

this sequence of commands

mkdir project && cd project && git init >/dev/null
npm init --yes >/dev/null
npm install ../commitplease
npm uninstall ../commitplease

calls install.js and uninstall.js properly. It does not remove the hook though.

With node --version at 6.3.0 and npm --version at 3.10.3 the same sequence of commands does not trigger uninstall.js at all.

With node --version at 6.1.0 and npm --version at 3.8.6 the same sequence of commands triggers install.js during npm uninstall ../commitplease, i.e. triggers the wrong script.

jzaefferer commented 8 years ago

According to https://docs.npmjs.com/misc/scripts the uninstall script should do what we expect.

I looked for open npm issues, this is the only one I've found: https://github.com/npm/npm/issues/10379 - its not about uninstalling, but since npm 3.x changed the behaviour of preinstall, maybe it also messed up uninstall.

If we can be sure that it is an npm issue, we should create a reduced test case (not involving commitplease) and file an issue against npm. Then document the manual workaround, since we can't expect this to get fixed anytime soon.

It does not remove the hook though.

Any idea why? What's the output?

alisianoi commented 8 years ago

Any idea why? What's the output?

It's the backticks. Now with npm --version at 2.15.x the hook gets removed just fine.

However, the problem with npm --version at 3.x.x remains. I have sketched a simple package.json that just echoes to console during install/uninstall. It produces the echo during install and stays silent during uninstall for both 3.8 and 3.10. My expectation was that 3.8 would produce the same output for install and uninstall (as it does with local install of commitplease). It does not. So, I will look further. You are right, it looks like an issue to npm is in order.

jzaefferer commented 8 years ago

Interesting. Apparently I had reviewed https://github.com/jzaefferer/commitplease/commit/6603f0fd607006ce99bef531b7a4354dceb1ce46 but never noticed the additional backticks. No idea why they were added in the first place, but it makes sense that it broke the uninstall script.

Let's file the issue for npm 3.x with a reduced testcase, then document the workaround (manual uninstall) here, along with a link to the npm issue.

jzaefferer commented 8 years ago

Looks good!