electron-userland / electron-userland/electron-builder

NodeModuleCopyHelper: filter() gets the parent dir, via a synchronous lstatSync, once per child entry

Open Beginner friendly
#10,169 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
14.7k
Forks
1.9k
Avg merge
3d 12h
Merged PRs (30d)
48

Description

### Version

app-builder-lib / electron-builder 26.15.7

### Summary

In `NodeModuleCopyHelper`, the per-file filter is called with the **parent directory** rather than the file, from inside a per-child async pool, using the **synchronous** `lstatSync`. So `fileMatched` is computed identically for every child of a directory, and the same path is `lstat`ed once per entry, synchronously, on the packaging hot path.

### Where

`src/util/NodeModuleCopyHelper.ts`, line 88:

```ts
const sortedFilePaths = await asyncPool(MAX_FILE_REQUESTS, childNames, async name => {
const filePath = path.join(dirPath, name)
const forceIncluded = onNodeModuleFile != null && !!onNodeModuleFile(filePath)
if (excludedFiles.has(name) || name.startsWith("._")) {
return null
}
// check if filematcher matches the files array as more important than the default excluded files.
const fileMatched = filter != null && filter(dirPath, lstatSync(dirPath)) // <-- dirPath, per child
if (!fileMatched || !forceIncluded || !!this.packager.config.disableDefaultIgnoredFiles) {
```

### Two separate problems in that line

**1. It passes `dirPath` where `filePath` looks intended.** `filePath` is computed two lines above and is what the surrounding code (`forceIncluded`, and every `name`-based check below) is about. As written, `filter` is asked about the directory, so `fileMatched` has the same value for every child of that directory — the comment above it says "check if filematcher matches the files array", which reads like a per-file decision that is not being made.

**2. `lstatSync` inside an async pool, re-stat'ing the same path.** `asyncPool(MAX_FILE_REQUESTS, childNames, …)` exists to overlap I/O; a synchronous `lstat` in the callback blocks the event loop instead, and it is the *same* `dirPath` on every iteration, so the work is also redundant. Notably `lstat` (async) is imported at the top of this very file — line 3, `import { lstat, lstatSync, readdir } from "fs-extra"` — and line 88 is the only use of either.

### Effect

This is on the `node_modules` copy path, which for us is the largest remaining phase of a Windows package: 33 s of a 98 s `yarn build` on `windows-latest` (electron-builder 26.15.7, ~9,000 archive entries). I have not isolated how much of that is this line, so I am reporting the code rather than claiming a number — the correctness half seems worth a look regardless of the cost.

### Suggested fix

```ts
const fileMatched = filter != null && filter(filePath, await lstat(filePath))
```

— or, if the parent really is the intended argument, hoist it out of the per-child callback so it is computed once per directory with the async `lstat`.

Happy to supply a profile or test a patch.

Contributor guide

Open the contributing guide

Research direction

Start in src/util/NodeModuleCopyHelper.ts at the asyncPool callback around line 88, and review the fs-extra imports and surrounding NodeModuleCopyHelper logic. Verify that filtering makes a per-child decision without repeating a synchronous parent-directory stat; done means the node_modules copy path preserves the intended filtering behavior without blocking or redundant stats.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
build-system
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
78/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.