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(deps): update husky package to the latest #403

Merged
merged 1 commit into from
Jan 12, 2022
Merged

fix(deps): update husky package to the latest #403

merged 1 commit into from
Jan 12, 2022

Conversation

XhmikosR
Copy link
Contributor

@XhmikosR XhmikosR commented Jan 8, 2022

🎉 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:

  • I have read the contribution guidelines.
  • The status checks are successful (continuous integration). Those can be seen below.

@erezrokah erezrokah added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Jan 10, 2022
@@ -60,10 +56,14 @@
"fp"
],
"license": "Apache-2.0",
"repository": "netlify/eslint-config-node",
"repository": {
Copy link
Contributor Author

@XhmikosR XhmikosR Jan 10, 2022

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.

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Jan 10, 2022

In order for this to apply, manual steps will be needed in your projects:

  1. remove husky from the upstream devDependencies
  2. create the needed changes to migrate

The second step can be done semi-automatically, there are instructions in the husky releases page which is what I used here myself.

@XhmikosR XhmikosR marked this pull request as ready for review January 10, 2022 17:26
@@ -0,0 +1,4 @@
#!/bin/sh
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

@XhmikosR XhmikosR Jan 10, 2022

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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 :)

Copy link
Contributor Author

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 :)

Copy link
Contributor

@ehmicky ehmicky Jan 10, 2022

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

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

@ehmicky ehmicky changed the title chore(deps): update husky package to the latest fix(deps): update husky package to the latest Jan 10, 2022
@ehmicky
Copy link
Contributor

ehmicky commented Jan 10, 2022

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.

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Jan 10, 2022 via email

@ehmicky ehmicky merged commit d056fa8 into netlify:main Jan 12, 2022
@XhmikosR XhmikosR deleted the update-husky branch January 12, 2022 18:08
This was referenced Jan 12, 2022
@ehmicky
Copy link
Contributor

ehmicky commented Jan 12, 2022

I have tried it in netlify/js-client#657, and there might be a few additional tweaks needed:

@XhmikosR
Copy link
Contributor Author

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 :)

@XhmikosR
Copy link
Contributor Author

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.

@ehmicky
Copy link
Contributor

ehmicky commented Jan 12, 2022

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 👍
The problem is that our bot is automatically merging the upgrade PRs, so we have to do this in separate PRs

I am currently trying to make it work on the netlify/js-client repository to see which are those changes, so we can then apply it on the other repositories 👍

@XhmikosR
Copy link
Contributor Author

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants