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

Rewrite npm's scripts in JavaScript #184

Merged
merged 1 commit into from May 14, 2021

Conversation

aminya
Copy link
Contributor

@aminya aminya commented May 9, 2021

  • This removes the dependency on sh for a Node env, which is not available on Windows.
  • It also speeds up the startup time because there is no sh script anymore to slow things down. Nodejs itself spawns the executable directly.

Fixes #183
Related to #16

This removes the dependency on `sh` for a Node env. `sh` is not available on Windows
@aminya aminya mentioned this pull request May 9, 2021
@aminya
Copy link
Contributor Author

aminya commented May 11, 2021

@DmitryTsepelev Could you take a look at this? It makes it possible to use lefthook on Windows.

@Envek
Copy link
Member

Envek commented May 11, 2021

@aminya hello and thanks for the pull request.

We're improving and maintaining Lefthook in the free time from commercial work, so we can't react to all contributions promptly.

We will get to your PR some day, but in the meantime please be patient and don't mention random people in the team. Thank you for understanding.

@aminya
Copy link
Contributor Author

aminya commented May 11, 2021

You're welcome. I am also contributing in my free time from my commercial work, and since the mentioned member approved two pull requests today, I added one kindly reminder. In the meantime, I am using my local fork, and I am not in rush at all.

@Envek Envek merged commit 42acef3 into evilmartians:master May 14, 2021
@Envek
Copy link
Member

Envek commented May 14, 2021

Windows support is especially hard to do right as we don't have anyone who is on Windows. That's why your contribution is especially valuable. @aminya, thank you very much!

@Envek
Copy link
Member

Envek commented May 14, 2021

Released in 0.7.5. Thank you!

@aminya
Copy link
Contributor Author

aminya commented May 15, 2021

You're welcome!

@aminya aminya deleted the windows-npm-postintall branch May 15, 2021 09:24
@PikachuEXE
Copy link
Contributor

This change (precisely the removal of .npm/bin/lefthook) causes #232.
Would be good to hear your opinion on how to fix #232. (Probably better to discuss there)

PikachuEXE added a commit to PikachuEXE/lefthook that referenced this pull request Sep 28, 2021
The old optimization was rewritten but left this conditional statement which does nothing now
evilmartians#184
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.

Fails to install on Windows npm, pnpm, or yarn
4 participants