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.

Sign up for GitHub

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

Jump to bottom

yarn conditional may be unreachable in hook template. #241

Closed
andrewhampton opened this issue Oct 11, 2021 · 3 comments · Fixed by #242
Closed

yarn conditional may be unreachable in hook template. #241

andrewhampton opened this issue Oct 11, 2021 · 3 comments · Fixed by #242

Comments

@andrewhampton
Copy link

The hook template currently contains this logic:

  elif npx @arkweid/lefthook -h >/dev/null 2>&1
  then
    npx @arkweid/lefthook $@
  elif yarn lefthook -h >/dev/null 2>&1
  then
    yarn lefthook $@

However, I'm not sure it's possible for the yarn branch to ever execute. I think, if yarn is installed, npx will almost certainly be installed. If npx is available on the machine, it will automatically download and run @arkweid/lefthook.

I think these branches should be swapped. The yarn branch should only work if lefthook is already installed locally. If it's not already available locally, falling back to npx seems like a good option.

Alternatively, npx has a --no-install flag that will prevent it from automatically installing lefthook.
@andrewhampton
Copy link
Author

Also, I'm happy to open a PR that implements this change if the maintainers would like to swap the two conditionals.

dannobytes added a commit to dannobytes/lefthook that referenced this issue Oct 11, 2021
@dannobytes
Fix unreachable conditional in hook template
ae56b8e 
Resolves evilmartians#241

For devs who are using yarn as their package manager, the git hook
templates are conditioned so that it will never attempt to run `yarn
lefthook` from the local package it is installed in. Instead, the
conditional will always end with `npx @arkweid/lefthook` which ends up
fetching the package via network.

In the worst case, the dev will fetch this package twice, once in the
`elif` conditional and again within the `then` block, causing a
significiant delay before the lefthook binary is executed. This degrades
performance to an almost unusable state.

To fix, give `yarn` a chance to find the lefthook binary within its
local cache before moving on to `npx`.
@dannobytes dannobytes mentioned this issue Oct 11, 2021
Merged
@dannobytes
Copy link
Contributor

Got a PR up with the suggested change, hopefully this helps.

mrexox pushed a commit that referenced this issue Mar 31, 2022
@dannobytes
Fix unreachable conditional in hook template (#242)
352531c 
Resolves #241

For devs who are using yarn as their package manager, the git hook
templates are conditioned so that it will never attempt to run `yarn
lefthook` from the local package it is installed in. Instead, the
conditional will always end with `npx @arkweid/lefthook` which ends up
fetching the package via network.

In the worst case, the dev will fetch this package twice, once in the
`elif` conditional and again within the `then` block, causing a
significiant delay before the lefthook binary is executed. This degrades
performance to an almost unusable state.

To fix, give `yarn` a chance to find the lefthook binary within its
local cache before moving on to `npx`.
@mrexox
Copy link
Member

mrexox commented Mar 31, 2022

Now in master, going to release it in v0.8.0

@defunctl defunctl mentioned this issue May 26, 2022
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix unreachable conditional in hook template dannobytes/lefthook
3 participants
@andrewhampton @dannobytes @mrexox