Optimize findManifestDependencies #247
Conversation
| const scripts = Object.entries(manifest.scripts ?? {}).reduce((scripts, [scriptName, script]) => { | ||
| if (script && (scriptFilter.length === 0 || scriptFilter.includes(scriptName))) { | ||
| return [...scripts, script]; | ||
| scripts.push(script); | ||
| } | ||
| return scripts; | ||
| }, [] as string[]); |
There was a problem hiding this comment.
FWIW, this is more or less what flatMap's made for
const scripts = Object.entries(manifest.scripts ?? {}).flatMap(([scriptName, script]) => {
if (script && (scriptFilter.length === 0 || scriptFilter.includes(scriptName))) {
return [script];
}
return [];
});There was a problem hiding this comment.
I think this code should be slower, because we still create new array.
| @@ -18,7 +18,7 @@ const findManifestDependencies = async ({ manifest, isProduction, isStrict, dir, | |||
|
|
|||
| const scripts = Object.entries(manifest.scripts ?? {}).reduce((scripts, [scriptName, script]) => { | |||
| if (script && (scriptFilter.length === 0 || scriptFilter.includes(scriptName))) { | |||
There was a problem hiding this comment.
If you wanted to micro-optimise this for allocations a bit further, you could move scriptFilter out of the function and make it a set so that it has faster .has() checks, and also is only allocated once. Then rather than conditionally setting it to either ['start', 'posinstall'] or [], move the isProduction check down here. So scriptFilter becomes const scriptFilter = new Set(['start', 'postinstall']); outside of the function, and this check becomes if (script && (!isProduction || scriptFilter.has(scriptName))) {
There was a problem hiding this comment.
I suggest making additions a bit later. I try not to make too many changes in the PR to reduce the likelihood of a bug. Moving the "isProduction" check is already a significant change, as it is already checked in another component. @webpro
There was a problem hiding this comment.
That's fine, but don't worry too much! We have tests, e.g. the isProduction flag with the start script has coverage.
|
Premature optimizations if you'd ask me (most time is spent during with disk IO and creating (and traversing) ASTs), it's still fun to implement and learn from each other here, so feel free to continue. I'm glad Knip receives also this kind of attention for detail! ❤️ I would prefer @me4502's suggestion, but tbh happy to merge any of the ideas here. |
|
Alright, merging this, thanks @Connormiha et al! |
|
🚀 This pull request is included in v2.26.0. See Release 2.26.0 for release notes. |
Spread creates a new array in each iteration, resulting in a complexity of O(n*n). Using
pushhas a complexity of O(n) and does not create a new array.