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

Fix unreachable conditional in hook template #242

Merged

Conversation

dannobytes
Copy link
Contributor

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.

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
Copy link
Contributor Author

@Envek Any initial feedback on getting this merged in? This addresses #241

@Envek
Copy link
Member

Envek commented Oct 28, 2021

Oh, all that yarn/npm/npx makes me mad. I will need to play with it a bit in a day or two. Sorry for the late reply.

@andrewhampton
Copy link

Thanks for the reply @Envek. I think the key point here is that the yarn branch will never execute as currently written. npx will download lefthook if it's not currently installed.

@mrexox
Copy link
Member

mrexox commented Mar 31, 2022

Great catch! Sorry for the late review.

@mrexox mrexox merged commit 352531c into evilmartians:master Mar 31, 2022
@dannobytes dannobytes deleted the fix-hook-template-unreachable-yarn branch April 18, 2022 20:59
mrexox added a commit that referenced this pull request Jun 9, 2022
* master:
  Account for GOAMD64 suffix in directory names in NPM and GEM packages [ci skip]
  Include version into RPM/DEB packages on release [ci skip]
  0.8.0: Skip hooks in merge/rebase, hide summary, NPM installer package
  Split NPM package to two: bundled and installer (#273)
  Include archived binaries in the releases (#189)
  docs: s/agrs/args (#265) [ci skip]
  chore(lint): Fix golangci-lint complains
  docs(usage): Add commitlint example in full_guide (#201)
  Fix unreachable conditional in hook template (#242)
  fix(hook.tmpl): adds cpu arch and os arch to `lefthook`'s filepath (#260)
  Replace deprecated `File.exists?` with `exist?` for Ruby wrapper (#263)
  Fix typo in docs/full_guide.md (#256)
  0.7.7: Fix arguments passing and various NPM-related fixes
  Fix incorrect npx command in git hook script template (#236)
  Update project URLs in NPM package.json (#235)
  Pass all arguments to downstream hooks (#231)
  Allows lefthook to work when node_modules is not in root folder for npx (#224)
  Do not initialize git config on `help` and `version` commands (#209)
  node: fix postinstall: process.cwd is a function and should be called

Signed-off-by: Valentin Kiselev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

yarn conditional may be unreachable in hook template.
4 participants