fix(hooks): run .hooks scripts even if package.json script is not present #13
Conversation
|
@zkat this should be good to land after your review. |
index.js
Outdated
| function hookExists (dir, stage) { | ||
| const hook = path.join(dir, '.hooks', stage) | ||
| if (_hookExistsCache[hook] === undefined) { | ||
| _hookExistsCache[hook] = fs.existsSync(hook) |
There was a problem hiding this comment.
I'd feel much more comfortable if this were an async operation. Also, fs.exists is deprecated in Node, so you'll have to use fs.stat with a catcher for err.code === 'ENOENT'.
There was a problem hiding this comment.
I think sync is fine here considering the cache may only get 3 or 4 values in it (opts.dir is constant, so the size of the cache is like 3 or 4), but happy to go async. I’ll do the change.
index.js
Outdated
| if (!pkg.scripts[stage]) return resolve() | ||
| // makeEnv is a slow operation. This guard clause prevents makeEnv being called | ||
| // and avoids a ton of unnecessary work, and results in a major perf boost. | ||
| if (!pkg.scripts[stage] && !hookExists(opts.dir, stage)) return resolve() |
There was a problem hiding this comment.
see above comment about async-ifying this.
index.js
Outdated
| @@ -16,6 +16,7 @@ const byline = require('byline') | |||
| const resolveFrom = require('resolve-from') | |||
|
|
|||
| const DEFAULT_NODE_GYP_PATH = resolveFrom(__dirname, 'node-gyp/bin/node-gyp') | |||
| const _hookExistsCache = {} | |||
There was a problem hiding this comment.
I'd rather this be a new Set(), since that's gonna be pretty efficient for this use-cases. There's a good chance we might be hammering this cache a lot.
There was a problem hiding this comment.
has that been benchmarked? for string keys, Object.create(null) might actually be faster than a Set.
There was a problem hiding this comment.
I need to cache positive and negative values, so Set seems inappropriate here. Also, the perf overhead here is peanuts, but I wouldn’t mind switching it out for Object.create(null) if that satisfies the micro optimization gods :-)
There was a problem hiding this comment.
Or I can use Map, but all-in-all I think any of the 3 choices is ok. Make the call and I’ll switch it.
There was a problem hiding this comment.
A Map is conceptually correct since you need more than just presence; a null object is likely to be faster based on recent benchmarks on a recent aphrodite PR - either seems fine to me, but I’d suggest not using a normal object.
Fixes: npm/npm#19258