Migrate from the glob package to Node's native fs.glob - #917
Conversation
Node's fsPromises.glob/fs.globSync became stable (no longer behind an
experimental flag/warning) in v22.17.0. With that as the new engines
floor, both of gscan's glob usages can move to the built-in and the
`glob` dependency can be dropped.
- Bump engines.node to ^22.17.0 || ^24.0.0, and the CI matrix's pinned
22.x version to match.
- lib/read-zip.js: replace `glob('**/index.hbs', {cwd, nosort: true})`
with `fs.glob()` + Array.fromAsync. The old call already made no
ordering guarantee (nosort: true), so for the edge case of a
malformed zip with more than one index.hbs, sort matches by path
depth (then alphabetically) and take the shallowest - deterministic
behavior where before it was whatever order the old library happened
to return, which is a strict improvement, not a behavior change to
rely on.
- test/valid-doc-links.js: replace `glob.sync('*.js', {ignore: ...})`
with `fs.globSync()` + a manual filter (the native `exclude` option
didn't behave as documented in local testing, so filtering after the
fact is simpler and doesn't depend on it).
- Remove `glob` from dependencies; yarn.lock updated.
- Expose read-zip's resolveBaseDir via module.exports._private and add
unit tests for the new depth-based sort and its alphabetical tiebreak
(no existing fixture covered the multi-match case at all).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reached
On-demand reviews are free for the next 17 days. After that, they cost $0.25 per reviewed file. Or wait 5 minutes for your next included review. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
Comment |
|
Review finding (P2): the new shallowest-match selection is not path-separator portable.
This matters for GScan’s standalone library/CLI usage on Windows even though the hosted service runs Linux. Suggested fix: calculate depth from a separator-neutral representation (e.g. split on |
The depth sort used .split('/') and the theme-root derivation manually
stripped a trailing '/', both assuming POSIX separators. fs.glob()
returns OS-native-separated paths, so on Windows this would compute
identical (wrong) depths for e.g. deep\nested\index.hbs and
shallow\index.hbs, and the trailing-separator strip would silently
no-op, potentially selecting the wrong theme root for a malformed zip
with more than one index.hbs.
- Compute depth on a separator-neutral split (/[\\/]/) instead of '/'.
- Derive the theme root via path.dirname()/path.join() (platform-
native) instead of a manual regex strip, so it stays correct for
whichever separator fs.glob actually returns on the running OS.
- Extract the sort comparator (compareMatchDepth) so it can be unit
tested directly with Windows-style ("\\") match strings on any OS,
independent of the platform-native path.dirname()/path.join() step.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Good catch — fixed in 5fb5bad. Depth is now computed on a separator-neutral split (/[\/]/) instead of '/', and the theme-root derivation uses path.dirname()/path.join() (platform-native) instead of the manual regex strip. Extracted the comparator (compareMatchDepth) so it's unit-testable directly with Windows-style ('\') match strings on any OS, independent of the platform-native path step. |
1 participant
What & why
Node's
fsPromises.glob/fs.globSyncbecame stable (no longer behind an experimental flag/warning) in v22.17.0. Bumps theengines.nodefloor to^22.17.0 || ^24.0.0and moves both of gscan'sglobusages to the built-in, dropping theglobdependency.Split out as its own PR (ahead of #909) per review discussion there - the Node-floor bump and dependency replacement stand alone, then the
readThemeStructurebounded-concurrency rewrite (addressing the unbounded directory-read fan-out flagged on #909) lands on top of this.Changes
engines.nodebumped, CI matrix's pinned 22.x version bumped to matchlib/read-zip.js:glob('**/index.hbs', {cwd, nosort: true})→fs.glob()+Array.fromAsync. The old call already made no ordering guarantee (nosort: true); for the edge case of a malformed zip with more than oneindex.hbs, matches are now sorted by path depth (then alphabetically) and the shallowest wins - deterministic where before it was whatever order the old library happened to return.test/valid-doc-links.js:glob.sync('*.js', {ignore: ...})→fs.globSync()+ a manual filter (the nativeexcludeoption didn't behave as documented in local testing).globremoved fromdependencies;yarn.lockupdated.resolveBaseDirexposed viamodule.exports._privateinread-zip.js, with new unit tests for the depth-sort and its tiebreak (no existing fixture covered the multi-match case).Testing
yarn test(437 tests, all green)yarn lintclean🤖 Generated with Claude Code