electron-userland / electron-userland/electron-builder
NodeModuleCopyHelper: filter() gets the parent dir, via a synchronous lstatSync, once per child entry
- 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
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