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

Husky pre-push hook adds dependency on yarn #716

Closed
lukebennett opened this issue Sep 26, 2020 · 2 comments · Fixed by blitz-js/blitz#1179
Closed

Husky pre-push hook adds dependency on yarn #716

lukebennett opened this issue Sep 26, 2020 · 2 comments · Fixed by blitz-js/blitz#1179

Comments

@lukebennett
Copy link
Contributor

What is the problem?

This change to the husky hooks assumes that yarn is installed: blitz-js/blitz#1104. Yet the docs don't stipulate yarn as a requirement, in fact the Getting Started guide uses npm - https://blitzjs.com/docs/getting-started.

As things stand, if you don't have yarn installed, the pre-push hook will error.

Steps to Reproduce

  1. Upgrade to Blitz v0.23.0
  2. Assuming husky is wired up (npm rebuild husky), attempt to push your branch to a remote
  3. The husky hook fails if yarn is not installed

Versions

Linux 4.4 | linux-x64 | Node: v12.11.0

blitz: 0.23.0 (global)
blitz: 0.23.0 (local)

  Package manager: npm 
  System:
    OS: Linux 4.4 Ubuntu 16.04.6 LTS (Xenial Xerus)
    CPU: (2) x64 Intel(R) Xeon(R) CPU           X5570  @ 2.93GHz
    Memory: 677.05 MB / 1.94 GB
    Shell: 4.3.48 - /bin/bash
  Binaries:
    Node: 12.11.0 - ~/.node/bin/node
    Yarn: Not Found
    npm: 6.14.5 - ~/.node/bin/npm
    Watchman: Not Found
  npmPackages:
    @prisma/cli: 2.7.1 => 2.7.1 
    @prisma/client: 2.7.1 => 2.7.1 
    blitz: 0.23.0 => 0.23.0 
    react: 0.0.0-experimental-7f28234f8 => 0.0.0-experimental-7f28234f8 
    react-dom: 0.0.0-experimental-7f28234f8 => 0.0.0-experimental-7f28234f8 
    typescript: 4.0.3 => 4.0.3 

Other

husky > pre-push (node v12.11.0)
sh: 1: yarn: not found
husky > pre-push hook failed (add --no-verify to bypass)

Whilst the fix for this shouldn't be complicated, there's a design decision needed on the preferred approach. If the preference is to stick with calling npm/yarn scripts, could look at using https://github.com/BendingBender/yarpm, or a combination of https://github.com/elijahmanor/cross-var and $npm_execpath. Alternatively it might be preferable to revert back to blitz test and avoid the issue of having to juggle between the two (I'm not sure what the rationale was in moving away).

@flybayer
Copy link
Member

Thanks @lukebennett Reason for the change was lint isn't ran by blitz test and it's not immediately clear to folks what blitz test does. Might end up removing that command at some point.


For now let's change from yarn lint && yarn test to npm lint && npm test. The result is the same an dit works for everyone.

Here's the line to change: https://github.com/blitz-js/blitz/blob/canary/packages/generator/templates/app/package.json#L25

@lukebennett
Copy link
Contributor Author

👍 PR opened, thanks

@dillondotzip dillondotzip transferred this issue from blitz-js/blitz Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants