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

Upgrade huksy and lint-staged dependencies #2319

Closed
wants to merge 1 commit into from
Closed

Upgrade huksy and lint-staged dependencies #2319

wants to merge 1 commit into from

Conversation

jooola
Copy link
Contributor

@jooola jooola commented Mar 11, 2021

Upgraded huksy and lint-staged dependencies with some extra changes :

  • Added subfolders and .ts files to the lint-staged glob pattern
  • New husky layout with .huksy folder and hooks inside

Copy link
Member

@kossnocorp kossnocorp left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution. Could you please describe in a paragraph what does this update provides and also address my comments?

package.json Outdated
@@ -20,22 +20,17 @@
"main": "index.js",
"module": "esm/index.js",
"scripts": {
"postinstall": "husky install",
Copy link
Member

Choose a reason for hiding this comment

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

What's the point of this? Won't it will affect all users installing date-fns?

I can see they recommend using pinst: https://typicode.github.io/husky/#/?id=yarn-v2

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 project seem to use Yarn v1, I replace the postinstall to the prepare lifecycle.

If yarn v2 is used, we will need to change this.

Copy link
Member

Choose a reason for hiding this comment

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

I mean users that install date-fns will have this command run and throw an error.

@@ -0,0 +1,4 @@
#!/bin/sh
. "$(dirname "$0")/_/husky.sh"
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if you can verify that it works, but could you please add support for Windows? (There're few paragraphs about that in the documentation).

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 added the small helper for windows, it should be working.

@jooola
Copy link
Contributor Author

jooola commented Apr 8, 2021

The main pros of the new versions are that husky is really lightweight and allow to run any kind of scripts (bash, node, python).

Other than that, it simply keep the project dependencies up to date.

@kossnocorp
Copy link
Member

@jooola got you, thanks, please fix the issue with postinstall.

@jooola
Copy link
Contributor Author

jooola commented Apr 14, 2021

@kossnocorp Fixed the postinstall issue. I guess we cannot enforce the use of husky on the developer side.

- Added subfolders files and ts files to the lint-staged glob pattern
- New husky layout with .huksy folder and hooks inside
- Using postinstall/prepare hooks for anything other than compilation seem to be a bad practise. See https://blog.typicode.com/husky-git-hooks-autoinstall/
@jooola
Copy link
Contributor Author

jooola commented Apr 18, 2022

Closing in favor of #2899

@jooola jooola closed this Apr 18, 2022
@jooola jooola deleted the upgrade_huksy_dependency branch April 18, 2022 20:01
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.

None yet

3 participants