refactor: remove glob #504

araujogui · 2025-12-02T22:45:02Z

Description

Removes glob dependency and create unit tests

Validation

https://gist.github.com/araujogui/3b3f8d4a597dce9615607a4b2b22660d

Related Issues

Check List

I have read the Contributing Guidelines and made commit messages that follow the guideline.
I have run node --run test and all tests passed.
I have check code formatting with node --run format & node --run lint.
I've covered new added functionality with unit tests if necessary.

vercel · 2025-12-02T22:45:07Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Updated (UTC)
api-docs-tooling	Ready	Preview	Dec 2, 2025 10:45pm

codecov · 2025-12-02T22:46:16Z

Codecov Report

❌ Patch coverage is 99.13793% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 75.13%. Comparing base (9285e74) to head (a04bbe8).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/generators/ast-js/index.mjs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #504      +/-   ##
==========================================
+ Coverage   74.60%   75.13%   +0.52%     
==========================================
  Files         112      114       +2     
  Lines       10700    10807     +107     
  Branches      722      736      +14     
==========================================
+ Hits         7983     8120     +137     
+ Misses       2714     2684      -30     
  Partials        3        3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

ovflowd

Do all LTS versions of Node now support glob?

avivkeller · 2025-12-02T22:47:33Z

src/loaders/javascript.mjs

-  const loadFiles = searchPath => {
-    const resolvedFiles = globSync(searchPath).filter(
+  const loadFiles = async searchPath => {
+    const resolvedFiles = globSync(searchPath);


nit: let's use the async variant

avivkeller · 2025-12-02T22:47:43Z

src/loaders/markdown.mjs

@@ -1,9 +1,9 @@
 'use strict';

+import { globSync } from 'node:fs';


avivkeller · 2025-12-02T22:49:00Z

Do all LTS versions of Node now support glob?

As of October, yes

Copilot

Pull request overview

This PR attempts to remove the external glob dependency and replace it with Node.js's native fs.globSync() function (available in Node.js v22+), while also adding unit tests for the loader modules and fixing an async/await bug in the JavaScript loader.

Key Changes:

Replaced import { globSync } from 'glob' with import { globSync } from 'node:fs'
Made loadFiles() functions properly async with Promise.all() to correctly handle concurrent file reads
Added JSDoc @returns {Promise<VFile[]>} documentation for return types
Added comprehensive unit tests for both markdown and JavaScript loaders
Removed glob package from dependencies

Critical Issue: The native Node.js fs.globSync() API is incompatible with the glob package's globSync() - specifically, the native version only accepts a single string pattern, while the code (and tests) are written to handle arrays of patterns. This will cause runtime failures in production.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
src/loaders/markdown.mjs	Replaced glob import with node:fs globSync and added return type documentation - has API incompatibility issues
src/loaders/javascript.mjs	Replaced glob import, fixed async/await Promise handling bug, added return type documentation - has API incompatibility issues
src/loaders/tests/markdown.test.mjs	New test file with mocked globSync - mock doesn't match real API
src/loaders/tests/javascript.test.mjs	New test file with mocked globSync
src/generators/ast-js/index.mjs	Added await for now-async loadFiles call
package.json	Removed glob dependency
npm-shrinkwrap.json	Removed glob and related transitive dependencies

Files not reviewed (1)

npm-shrinkwrap.json: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-02T22:49:50Z

src/loaders/markdown.mjs

@@ -1,9 +1,9 @@
 'use strict';

+import { globSync } from 'node:fs';


The import { globSync } from 'node:fs' is only available in Node.js v22.0.0 and later. While the project uses Node.js 22 per .nvmrc, consider documenting this Node.js version requirement in the package.json engines field to prevent issues for users on older versions:

"engines": { "node": ">=22.0.0" }

Copilot · 2025-12-02T22:49:51Z

src/loaders/__tests__/markdown.test.mjs

+    globSync: (/** @type {string[]} */ path) => {
+      if (isDeepStrictEqual(path, ['ignore.md'])) {
+        return ['/fake/path/file2.md'];
+      }
+      return [
+        '/fake/path/file1.md',
+        '/fake/path/file2.md',
+        '/fake/path/not-a-md-file.txt',
+      ];
+    },
+  },


The mock implementation accepts an array parameter (string[]), but the native Node.js fs.globSync() only accepts a single string pattern, not an array. This mock doesn't match the actual API behavior, which could hide bugs. The mock should match the real API signature:

globSync: (pattern) => { if (pattern === 'ignore.md') { return ['/fake/path/file2.md']; } // handle string pattern return [...]; }

Also, the test on line 42 calls loadFiles(['*.md']) with an array, but this won't work with the real fs.globSync implementation.

Copilot · 2025-12-02T22:49:51Z

src/loaders/__tests__/markdown.test.mjs

+    ];
+
+    const loader = createLoader();
+    const vfiles = await loader.loadFiles(['*.md']);


Test calls loadFiles(['*.md']) with an array, but according to the JSDoc in the source, searchPath should be Array<string>. However, the native Node.js fs.globSync() only accepts a single string pattern, not an array. This test would pass with the mock but fail in production. Consider using a string pattern instead: loadFiles('*.md')

Suggested change

const vfiles = await loader.loadFiles(['*.md']);

const vfiles = await loader.loadFiles('*.md');

Copilot · 2025-12-02T22:49:51Z

src/loaders/__tests__/markdown.test.mjs

+
+  it('should ignore specified files', async () => {
+    const loader = createLoader();
+    const vfiles = await loader.loadFiles(['*.md'], ['ignore.md']);


Test calls loadFiles(['*.md'], ['ignore.md']) with arrays, but the native Node.js fs.globSync() only accepts a single string pattern, not an array. This test would pass with the mock but fail in production. Consider using string patterns instead: loadFiles('*.md', 'ignore.md')

Copilot · 2025-12-02T22:49:51Z

src/loaders/markdown.mjs

   * Loads API Doc files and transforms it into VFiles
   *
   * @param {Array<string>} searchPath A glob/path for API docs to be loaded
   * @param {Array<string> | undefined} [ignorePath] A glob/path of files to ignore


The JSDoc indicates searchPath is Array<string>, but the native Node.js fs.globSync() expects a string pattern as its first argument, not an array (unlike the glob package which could handle arrays). This will cause a runtime error when an array is passed to loadFiles().

Consider either:

Updating the JSDoc to reflect that searchPath should be a string: @param {string} searchPath

Or updating the implementation to handle arrays by iterating over patterns

Copilot · 2025-12-02T22:49:52Z

src/loaders/javascript.mjs

-  const loadFiles = searchPath => {
-    const resolvedFiles = globSync(searchPath).filter(
+  const loadFiles = async searchPath => {
+    const resolvedFiles = globSync(searchPath);


Similar issue: The JSDoc indicates searchPath can be string | Array<string>, but the native Node.js fs.globSync() expects only a string pattern as its first argument (unlike the glob package). When an array is passed, it will cause a runtime error.

Consider either:

Updating the JSDoc to only accept string: @param {string} searchPath

Or updating the implementation to handle arrays by iterating over patterns

Suggested change

const resolvedFiles = globSync(searchPath);

let resolvedFiles = [];

if (Array.isArray(searchPath)) {

for (const pattern of searchPath) {

resolvedFiles.push(...globSync(pattern));

}

} else {

resolvedFiles = globSync(searchPath);

}

richardlau · 2025-12-02T23:07:27Z

Do all LTS versions of Node now support glob?

As of October, yes

Node.js 20 does not.

ovflowd · 2025-12-02T23:39:36Z

Do all LTS versions of Node now support glob?

As of October, yes

Node.js 20 does not.

Indeed, Node.js 20 is not EoL yet

avivkeller · 2025-12-03T22:31:37Z

Ahh, I thought it did, apologies

araujogui · 2025-12-04T16:38:07Z

Do all LTS versions of Node now support glob?

As of October, yes

Node.js 20 does not.

Indeed, Node.js 20 is not EoL yet

Uh, should I close this PR then?

araujogui added 4 commits December 2, 2025 19:30

refactor: remove glob

747d650

test(loaders): create unit tests

d33209b

fix: add missing await

61beafa

chore: remove ts directive

a04bbe8

araujogui requested a review from a team as a code owner December 2, 2025 22:45

Copilot AI review requested due to automatic review settings December 2, 2025 22:45

Copilot started reviewing on behalf of araujogui December 2, 2025 22:45 View session

ovflowd reviewed Dec 2, 2025

View reviewed changes

avivkeller approved these changes Dec 2, 2025

View reviewed changes

Copilot finished reviewing on behalf of araujogui December 2, 2025 22:48

Copilot AI reviewed Dec 2, 2025

View reviewed changes

AugustinMauroy approved these changes Dec 3, 2025

View reviewed changes

		@@ -1,9 +1,9 @@
		'use strict';

		import { globSync } from 'node:fs';

	const vfiles = await loader.loadFiles(['*.md']);
	const vfiles = await loader.loadFiles('*.md');

-    const resolvedFiles = globSync(searchPath);
+    let resolvedFiles = [];
+    if (Array.isArray(searchPath)) {
+      for (const pattern of searchPath) {
+        resolvedFiles.push(...globSync(pattern));
+      }
+    } else {
+      resolvedFiles = globSync(searchPath);
+    }

refactor: remove glob #504

Are you sure you want to change the base?

refactor: remove glob #504

Uh oh!

Conversation

araujogui commented Dec 2, 2025

Description

Validation

Related Issues

Check List

Uh oh!

vercel bot commented Dec 2, 2025

Uh oh!

codecov bot commented Dec 2, 2025

Codecov Report

Uh oh!

ovflowd left a comment

Choose a reason for hiding this comment

Uh oh!

avivkeller Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

avivkeller Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

avivkeller commented Dec 2, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

richardlau commented Dec 2, 2025

Uh oh!

ovflowd commented Dec 2, 2025

Uh oh!

avivkeller commented Dec 3, 2025

Uh oh!

araujogui commented Dec 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants