Skip to content

[Question] Why is post-commit part of suggested setup? #20

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

Closed
Svish opened this issue Dec 5, 2019 · 3 comments
Closed

[Question] Why is post-commit part of suggested setup? #20

Svish opened this issue Dec 5, 2019 · 3 comments
Assignees
Labels
question Further information is requested
Milestone

Comments

@Svish
Copy link

Svish commented Dec 5, 2019

Just wondering why the post-commit hook is part of the suggested setup. I understand the other 3, but, yeah... 🤔

From what I can understand, the setup checks package-lock.json for changes and runs npm install if it has any. Makes sense for the other 3, but in what cases would package-lock.json change without npm install having been run already as part of a commit? For example if I upgrade or add a package, I run npm install new-package, which updates the package.json and package-lock.json files, and then I run git commit to commit that change.

What's the use of running npm install again after that as part of the post-commit hook? Or is there a case I'm not thinking of here?

@Svish Svish changed the title Why is post-commit part of suggested setup? [Question] Why is post-commit part of suggested setup? Dec 5, 2019
@hkdobrev
Copy link
Owner

hkdobrev commented Dec 9, 2019

@Svish Thanks for your question!

It is true that in the scenario you describe, the additional install run is unnecessary.

However, there are other cases like when using git cherry-pick where post-commit hook could occur. So this is just out of precaution as I've found people often miss some edge cases and end up working with an outdated set of dependencies without realising it.

@Svish
Copy link
Author

Svish commented Dec 9, 2019

Ah, ok, that makes sense.

I do find it kind of annoying that it runs npm install when I've already done it before the commit (as part of updating package.json) though. How does it currently check if package-lock.json has changed? Would it be possible to improve this diff check somehow, or would that maybe be not so easy? 🤔

@hkdobrev
Copy link
Owner

@Svish The current logic is in this git command:

function getFilesFromGit() {
const { stdout } = execa.sync('git', [
'diff-tree',
'--name-only',
'--no-commit-id',
'-r',
'HEAD@{1}',
'HEAD',
]);
return stdout;
}

So it runs:

git diff-tree --name-only --no-commit-id -r HEAD@{1} HEAD

This would return a newline-separated list of pathnames that are changed between the previous and the current state of the HEAD.
So if previous committed/picked state had one package-lock.json and the last/current state had another, it would include the file in the list and match it against the configuration.

Everything is around the HEAD@{1} HEAD range based on relative changes to HEAD.


I'm not sure how you would imagine changing that check, but if you can provide some ideas I can help with the git part.


I think a different solution would be to reduce the cost of the unnecessary npm install (or other operations). So still run the configured command, but do a really cheap (quick & silent) check on whether the real command is necessary. E.g. if the package manager has some kind of a lock/cache/hash which would tell you if the installed dependencies are synced. Not sure if npm install is already doing that.

And if the problem is not about the speed, we can just hide the output when not doing anything substantial.

@hkdobrev hkdobrev added the question Further information is requested label Dec 11, 2019
@hkdobrev hkdobrev self-assigned this Dec 11, 2019
@hkdobrev hkdobrev added this to the 0.x milestone Dec 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants