Fixed calling hooks in node_modules/.hooks folder

[ties-s]

Fixed not calling hooks in node_modules/.hooks when there is no script for that event in package.json.

Fixes: npm/npm#19258

[zkat]

@ties-s I feel like I'd like this to be a bigger refactor -- the point of this shortcut is to avoid unnecessary fs.stat() and other filesystem calls when we already know a package won't need any hooks running. This is definitely not the only place this shortcut exists in npm right now, so it might be good to be more organized and inject hooks as options/extra args in some way, such that we only ever do one stat hit to node_modules/.hooks, at startup time (of either npm i or npm ci), and this can continue working.

/cc @mikesherov who might be more interested in pulling off the larger refactor.

I'm gonna hold off on this PR right now, because it won't fully fix the problem, and I'd rather have this working completely.

[mikesherov]

Thanks for contributing!

Yeah, that line being removed is an important speed optimization. Luckily, most of the slowdown from going past that point is from the makeEnv call. If you changed the guard to also do an fs.stat call, we retain speed and restore correctness.

[ties-s]

@zkat it might be in more places but removing that line fixed the problem in my case.

@mikesherov I added an extra guard in the form of fs.existsSync. Please let me know if that is what you meant or if I should make it an asynchronous call.

[mikesherov]

@ties-s that looks better. Thanks for sticking with this. However, I’m wondering how this affects performance and if we need a cache for the result of these lookups.

Can you run time npm i a few times with and without this change and post results?

Also, it seems like we should have a unit test for this case. Can you kindly add one?

[ties-s]

@mikesherov Timed on 2 brand new virtual machines.

Hardware: MacBook Pro 2016, 2,7 GHz Intel Core i7, 16GB RAM
OS: Ubuntu 16.04
Node: v8.10.0
NPM: v5.6.0

Using angular sample project as test material as it has a nice long list of dependencies.

Results:
current version of npm-lifecycle

added 486 packages in 23.759s

real 0m24.390s
user 0m21.048s
sys 0m3.268s

with extra guard

added 486 packages in 25.147s

real 0m25.471s
user 0m21.056s
sys 0m3.260s

So it is little bit slower but I think thats acceptable if it makes it work like the documentation says.

[mikesherov]

@ties-s thanks for that, and sticking with it.

2 more requests before this can land:

1. Can you add a unit test for this change?
2. Can you also show times with the guard removed completely? I just want to know how far of a reversion this is in terms of speed, because I think we can cache the result of path.join(opts.dir, ‘hooks’); (i.e. without the stage appended) and use it as an additional check before checking for the stage specific hook, which should hopefully mitigate this completely for projects that don’t have hooks (which is an overwhelming majority).

[mikesherov]

Hi @ties-s I took a crack at fixing this issue too, and came up with this: #13

I think I'm going to land that patch, but happy to give you credit for the commit if you'd like. Please let me know! Thanks again!

[ties-s]

@mikesherov looks good. I don’t need credit. I’m happy when this is fixed. Do you have any idea when a npm version with this change might be released?

[mikesherov]

@zkat can let us know when this’ll land in npm proper. Thanks again for your help!