perf: cli/lookup-files.js, refactor, add jsdoc, reduce intermediary variables #5489

Uzlopak · 2025-10-03T04:51:57Z

lookupFiles repeats various operations over and over. We can avoid alot of this overhead and improve the performance of big test suites.

To improve the performance, we normalize the file extensions once at the beginning and dont need to do it again.

This means that we can avoid again normalizing the array in hasMatchingFileExtension (before: hasMatchingExtname). Also we avoid the performance bottleneck of Array.helper functions like .map and .some().

fs.statSync is called with throwIfNoEntry, which exists since node 14.17.0, set to false. We can avoid "expensive" errors and the try catch blocks. We just check if stat is undefined, and if so, well, the path has an issue.

Using recursive and withFileTypes option of readdirsync, we can avoid using recursive calling of lookupFiles.

Added jsdoc for better type resolution. Also if you would put // @ts-check at the beginning of the file, it would pass.

Copilot

Pull Request Overview

This PR refactors the lookupFiles function to improve performance by reducing redundant operations and intermediate variables. The main optimization involves normalizing file extensions once at the beginning rather than repeatedly during recursive calls.

Extracted normalizeFileExtensions function to handle extension normalization once per run
Replaced array helper methods with manual loops to improve performance
Used fs.statSync with throwIfNoEntry: false to avoid expensive try-catch blocks

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

lib/cli/lookup-files.js

…alot of this overhead and improve the performance of big test suites. To improve the performance, we normalize the file extensions once at the beginning and dont need to do it again. This means that we can avoid again normalizing the array in `hasMatchingFileExtension` (before: `hasMatchingExtname`). Also we avoid the performance bottleneck of Array.helper functions like `.map` and `.some()`. `fs.statSync` is called with `throwIfNoEntry`, which exists since node 14.17.0, set to `false`. We can avoid "expensive" errors and the try catch blocks. We just check if stat is undefined, and if so, well, the path has an issue. Using `recursive` and `withFileTypes` option of readdirsync, we can avoid using recursive calling of `lookupFiles`. Added `jsdoc` for better type resolution. Also if you would put `// @ts-check` at the beginning of the file, it would pass.

JoshuaKGoldberg

I like the idea of this, but before adding & changing logic for a performance need, let's validate the need?

JoshuaKGoldberg · 2025-10-03T11:44:01Z

lib/cli/lookup-files.js

performance

@Uzlopak this looks like a lot of good optimizations, but I'm not aware of any known performance woes with file lookups. Can you demonstrate that there are issues for users? A reproduction case + measurement script, etc.?

This is my main concern as well. Reviewing a PR like this takes a lot of time. I don't want this work to languish but I don't want to ignore potentially bigger issues with Mocha (pipeline failures, known bugs, easy requested features, etc.) I think we should follow the perf issue template and discuss before taking up significant work like this so we're on the same page with review timelines and priority

JoshuaKGoldberg · 2025-10-03T11:45:12Z

lib/cli/lookup-files.js

+const {
+  createNoFilesMatchPatternError,
+  createMissingArgumentError
+} = require('../errors');


Can you separate out these unrelated changes, like var -> const? I'm in favor of them in general but the diff is already pretty big.

If you want to holistically change most/all vars to consts separately I'd be very in favor.

mark-wiemer

First pass looks good, I'll have to review code coverage and triple check things before I'm comfortable signing off though

mark-wiemer · 2025-10-04T16:46:54Z

lib/cli/lookup-files.js

+    return [];
+  }
+
+  for (var i = 0; i < exts.length; i++) {


Suggested change

for (var i = 0; i < exts.length; i++) {

for (let i = 0; i < exts.length; i++) {

mark-wiemer · 2025-10-04T16:48:20Z

lib/cli/lookup-files.js

+ * @param {FileExtension[]|string[]|undefined|null} exts
+ * @returns {FileExtension[]}
+ */
+const normalizeFileExtensions = (exts) => {


is the expected behavior for normalizeFileExtensions(['hello.world']) really to return [.hello.world']? extensions should just be the stuff after the last . . Maybe we are careful about this in the callers but it's something to consider.

That said, I haven't read the rest of the PR yet!

mark-wiemer · 2025-10-04T16:49:28Z

lib/cli/lookup-files.js

 module.exports = function lookupFiles(
  filepath,
-  extensions = [],
+  fileExtensions,


let's make the code match the comment. I know normalizeFileExtensions does this for us but explicit is a bit more readable

Suggested change

fileExtensions,

fileExtensions = [],

mark-wiemer · 2025-10-04T16:52:01Z

lib/cli/lookup-files.js

+    // Handle directory
+    const dirEnts = fs.readdirSync(filepath, { recursive, withFileTypes: true });
+
+    for (var i = 0; i < dirEnts.length; i++) {


Suggested change

for (var i = 0; i < dirEnts.length; i++) {

for (const dirEnt of dirEnts) {

mark-wiemer · 2025-10-04T16:54:23Z

lib/cli/lookup-files.js

+      const pathname = dirEnt.parentPath
+        ? path.join(dirEnt.parentPath, dirEnt.name)
+        : path.join(filepath, dirEnt.name);


Suggested change

const pathname = dirEnt.parentPath

? path.join(dirEnt.parentPath, dirEnt.name)

: path.join(filepath, dirEnt.name);

const pathToJoin = dirEnt.parentPath ?? filepath

const pathname = path.join(pathToJoin, dirEnt.name)

mark-wiemer · 2025-10-04T16:57:24Z

lib/cli/lookup-files.js

This is my main concern as well. Reviewing a PR like this takes a lot of time. I don't want this work to languish but I don't want to ignore potentially bigger issues with Mocha (pipeline failures, known bugs, easy requested features, etc.) I think we should follow the perf issue template and discuss before taking up significant work like this so we're on the same page with review timelines and priority

This comment was marked as off-topic.

Sign in to view

Uzlopak requested a review from Copilot October 3, 2025 05:01

Copilot AI reviewed Oct 3, 2025

View reviewed changes

Uzlopak force-pushed the perf-lookup-files branch 3 times, most recently from a8f7451 to 918523e Compare October 3, 2025 06:35

Uzlopak force-pushed the perf-lookup-files branch from 102cc3c to 145344c Compare October 3, 2025 07:10

JoshuaKGoldberg requested changes Oct 3, 2025

View reviewed changes

JoshuaKGoldberg added the status: waiting for author waiting on response from OP or other posters - more information needed label Oct 3, 2025

mark-wiemer reviewed Oct 4, 2025

View reviewed changes

	for (var i = 0; i < exts.length; i++) {
	for (let i = 0; i < exts.length; i++) {

	for (var i = 0; i < dirEnts.length; i++) {
	for (const dirEnt of dirEnts) {

Uh oh!

perf: cli/lookup-files.js, refactor, add jsdoc, reduce intermediary variables #5489

Are you sure you want to change the base?

perf: cli/lookup-files.js, refactor, add jsdoc, reduce intermediary variables #5489

Uh oh!

Conversation

Uzlopak commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as off-topic.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mark-wiemer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uzlopak commented Oct 3, 2025 •

edited

Loading