Skip to content

Commit

Permalink
Fix unreachable conditional in hook template (#242)
Browse files Browse the repository at this point in the history
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`.
  • Loading branch information
dannobytes authored Mar 31, 2022
1 parent c43d472 commit 352531c
Showing 1 changed file with 3 additions and 3 deletions.
6 changes: 3 additions & 3 deletions cmd/templates/hook.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,12 @@ call_lefthook()
elif bundle exec lefthook -h >/dev/null 2>&1
then
bundle exec lefthook $@
elif npx @arkweid/lefthook -h >/dev/null 2>&1
then
npx @arkweid/lefthook $@
elif yarn lefthook -h >/dev/null 2>&1
then
yarn lefthook $@
elif npx @arkweid/lefthook -h >/dev/null 2>&1
then
npx @arkweid/lefthook $@
else
echo "Can't find lefthook in PATH"
fi
Expand Down

4 comments on commit 352531c

@KubaJastrz
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dannobytes when can we expect it to be released? it's blocking my team from using lefthook

@dannobytes
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dannobytes when can we expect it to be released? it's blocking my team from using lefthook

Not my call, but looks like they'll be releasing it soon
#241 (comment)

@KubaJastrz
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry, I meant to ask @mrexox not you 😅

@sbleon
Copy link

@sbleon sbleon commented on 352531c Jun 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixed #262 . Thanks!

Please sign in to comment.