Should not exit with error when ghooks is missing
joaomoreno opened this issue · 9 comments
dcc01d2 introduced an error return value for when ghooks is not found.
This is a really bad idea, since git commands start failing whenever the node_modules directory is not populated. This started breaking our (Microsoft/vscode) integration.
I believe the previous behaviour of warning the user was the best one.
cc @dbaeumer
+1
and I found my gitlab ci build fail today because of this change.
We could think about ways to make exiting with an error a configurable option (taken into account on install, maybe as a "re-runable" update script?).
Yeah, in retrospect, maybe this feature should have been released behind a flag so it doesn't break for people relying on the previous "default". On the other hand, and this can be very context specific, it could be potentially harmful to have ghooks installed, expecting hooks to run, but ignoring the fact that they where not actually running…
On a separate note, but somewhat related to this issue, one thing that just occurred me is that if we embed lib/runner.js within the template that gets dumped into each hook, we would not need ghooks to actually exist in node_modules, because all the logic to read from package.json, and so on, would be in the hook itself. Maybe this is a really bad idea, idk…
Patching the logic into the hooks themselves sounds good for me.
Couldn't we alternatively put the lib/runner.js into a separate file in the hooks folder, then being required by the hooks?
Updating ghooks as a dependency might then reinvoke the patching procedure to update the runner logic.
I like it. This way the hooks themselves don't get too cluttered, with too many responsibilities.
Going down this route, though, would have the hooks running regardless of ghooks being under node_modules or not, which is still different than the "default" behavior pre 1.1.0. If users where relying simply on the warning, this could represent an issue for them.
@joaomoreno, @bruceCzK any 💭?
Love that idea.
I wanted to weigh in that this new behavior is really painful on a project where I've recently added ghooks, but developers are still working on branches that don't yet have it. For "reasons," they can't rebase on the branch that has ghooks installed, but if they've visited that ghooks branch and run npm install, then they have these unskippable hooks on their feature branch. I expected -n to skip git hooks like it says on the tin -- indeed, it even says "Skipping git hooks" -- so this new behavior is incredibly unexpected.
As a workaround, I've had these developers npm install ghooks on their branch, without --save or --save-dev, so they can continue working. On a team that isn't accustomed to Node and npm land, though, this definitely isn't ideal.
This should be fixed now. Could y'all try out the latest version and let us know please? Thank you for your patience :-)