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

Should not exit with error when ghooks is missing #70

Closed
joaomoreno opened this issue Mar 31, 2016 · 9 comments · Fixed by #80
Closed

Should not exit with error when ghooks is missing #70

joaomoreno opened this issue Mar 31, 2016 · 9 comments · Fixed by #80

Comments

@joaomoreno
Copy link

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.

@joaomoreno
Copy link
Author

cc @dbaeumer

@bruceCzK
Copy link

+1
and I found my gitlab ci build fail today because of this change.

@ta2edchimp
Copy link
Collaborator

@kentcdodds @gtramontina:

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?).

@gtramontina
Copy link
Collaborator

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…

@ta2edchimp
Copy link
Collaborator

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.

@gtramontina
Copy link
Collaborator

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 💭?

@joaomoreno
Copy link
Author

Love that idea.

@rmurphey
Copy link

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.

@kentcdodds
Copy link
Contributor

This should be fixed now. Could y'all try out the latest version and let us know please? Thank you for your patience :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

6 participants