-
Notifications
You must be signed in to change notification settings - Fork 4
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(deps): update husky package to the latest #403
Conversation
@@ -60,10 +56,14 @@ | |||
"fp" | |||
], | |||
"license": "Apache-2.0", | |||
"repository": "netlify/eslint-config-node", | |||
"repository": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are automatically added by npm init
. We can still just use "repository": "netlify/eslint-config-node"
but the bugs
and homepage
properties are redundant. Let me know which one you prefer. I personally just go with the explicit npm init
properties but both should have the same effect.
In order for this to apply, manual steps will be needed in your projects:
The second step can be done semi-automatically, there are instructions in the husky releases page which is what I used here myself. |
@@ -0,0 +1,4 @@ | |||
#!/bin/sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this is uncommon, some Windows systems are lacking sh
. Is there a way to use Husky v7 for those?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm actually on Windows mostly and it works fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most Windows developers have sh
installed, but not all.
Sometimes, an alternative is to use a Node script instead. However, I am wondering whether this is possible with Husky v7?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unrelated to bash, I don't have bash at all. The changes you see here are from their official migration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, these are git hooks, which work fine everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, basically husky sets the core.hooksPath
of the git repo (when doing npm install
) to be ./.husky
, hence how this works :)
EDIT: git for Windows ships with its own MSYS2 system which includes bash, to answer your question about bash. So, anyone who's installed Git on Windows does have everything set up. I'm on Windows myself (mostly) and this works fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that most Windows developers have sh
or Bash installed through git for Windows or other similar solutions. However, I have encountered some Windows developers who do not. While this is uncommon, I am wondering whether there is a way to make it work for them.
That being said, if Husky v7 does require sh
, then this change sounds good to me.
What are your thoughts on this @erezrokah?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ehmicky please read my comments :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not most, everyone who installs Git on Windows has MSYS2 -> bash. That's how Git works on Windows. This is nothing new, it's like that forever.
So, what you are seeing here is simply git hooks. How they are handled it's outside of the scope of the PR but I explained that above :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @XhmikosR, I forgot that installing git
implied installing Bash as well. You're absolutely correct.
package.json
Outdated
], | ||
"bin": { | ||
"run-e": "./bin/run_e.js", | ||
"run-ci": "./bin/run_ci.js", | ||
"run-local": "./bin/run_local.js" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In most of our repositories, we use exports
, which requires file paths to start with ./
. While removing in bin
is possible, we might want to keep it for consistency. Also, this change might be unrelated to Husky?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done automatically by npm init -y
. I can revert it, but I'd trust npm more here :)
But yeah it's unrelated to the husky update, I just ran npm init -y
in this branch by mistake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
npm init
should not be needed anymore for this repository, since it's already been initialized, so this should not be a problem for contributors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted the change already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This looks good, thanks! |
Can't you change it when squashing and merging? I'm away from my main
machine right now
…On Mon, Jan 10, 2022, 20:35 ehmicky ***@***.***> wrote:
This looks good, thanks!
Would you like to please change the commit message from chore: to fix:?
This will ensure our release-please bot creates a new release for this
change.
—
Reply to this email directly, view it on GitHub
<#403 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACVLNJLNLSSKGOFIHH5GFDUVMRHHANCNFSM5LQO6BYA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I have tried it in netlify/js-client#657, and there might be a few additional tweaks needed: |
Sorry, about that! It worked fine for this project here and I thought it was OK. Feel free to revert the update if it's too much trouble :) PS. I'm on Windows so I couldn't have noticed the permission issue :) |
Also, to clarify, someone will need to make the related changes to each project, i.e. it's not just a simple update. I should have communicated this better. So, this should have been a major version bump probably. |
No worries @XhmikosR! We needed to make that upgrade, so this is very helpful. We just need to make a few tweaks and it will work. Also, I believe the old Husky setup still seems to work until the changes you are mentioning are performed. You also did communicate the changes to be done 👍 I am currently trying to make it work on the |
Thanks for your patience, I misjudged the upgrade for sure. Ideally, you should update but it looks like it might be a lot of trouble. I'd be curious to see one project migrated. And don't feel bad reverting this PR if needed. I'd at least make it a major bump due to the manual work required :) |
🎉 Thanks for sending this pull request! 🎉
Please make sure the title is clear and descriptive.
If you are fixing a typo or documentation, please skip these instructions.
Otherwise please fill in the sections below.
Which problem is this pull request solving?
Example: I'm always frustrated when [...]
List other issues or pull requests related to this problem
Example: This fixes #5012
Describe the solution you've chosen
Example: I've fixed this by [...]
Describe alternatives you've considered
Example: Another solution would be [...]
Checklist
Please add a
x
inside each checkbox: