fix: replace glob npm package with native node:fs/promises glob#52
Conversation
Migrate pagesRouter, apiRouter, and appRouter from the third-party 'glob'
package to Node.js built-in glob (node:fs/promises), available since
Node 22. This removes a dependency and fixes path handling issues on
Windows where dynamic route segments (e.g. [slug]) were incorrectly
parsed due to premature path separator normalization.
Changes:
- Use for-await with glob() from node:fs/promises
- Use exclude callbacks instead of post-filtering
- Normalize pagePath/routePath with replaceAll('\\', '/') in output
- Remove glob dependency from package.json
There was a problem hiding this comment.
Pull request overview
This PR migrates vinext’s routing scanners from the third-party glob npm package to Node’s built-in glob() API (node:fs/promises), aligning with Node 22+ and removing an external dependency.
Changes:
- Replace
globpackage usage inpagesRouter/apiRouterandappRouterwithfor awaititeration overnode:fs/promisesglob(). - Move
@slotexclusion into the nativeglob()excludecallback for App Router scanning. - Remove the
globdependency frompackages/vinext/package.json(and update lockfile accordingly).
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pnpm-lock.yaml | Drops glob and its transitive dependencies from the lockfile. |
| packages/vinext/src/routing/pages-router.ts | Uses native glob() async iteration and updates filtering logic. |
| packages/vinext/src/routing/app-router.ts | Uses native glob() async iteration, excludes @slot paths via exclude, normalizes returned pagePath/routePath. |
| packages/vinext/package.json | Removes glob dependency. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
packages/vinext/package.json:63
- This change switches routing to
node:fs/promises'glob(), which is only available starting in Node 22. The package currently doesn’t declare a Node engine requirement, so consumers running older Node versions will hit a runtime import error. Please add an explicitengines.node(e.g.>=22) for thevinextpackage (and/or the monorepo) or provide a backwards-compatible fallback implementation.
"dependencies": {
"@unpic/react": "^1.0.2",
"@vercel/og": "^0.8.6",
"@vitejs/plugin-rsc": "^0.5.19",
"magic-string": "^0.30.21",
"react-server-dom-webpack": "^19.2.4",
"rsc-html-stream": "^0.0.7",
"vite-tsconfig-paths": "^6.1.1"
},
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/bigbonk review this please |
commit: |
There was a problem hiding this comment.
Review: Replace glob npm package with native node:fs/promises glob
Good direction — removing the glob dependency is worthwhile. The implementation has one behavioral regression that needs fixing, plus a few minor points.
Behavioral regression: excludePrivate over-excludes
The exclude callback in node:fs/promises glob() is called on each filesystem entry during the walk, at every depth. name.startsWith("_") will exclude _-prefixed files and directories inside subdirectories (e.g., pages/nested/_helper.tsx), whereas the old ignore: ["_*"] only matched at the cwd root. Similarly, name === "api" will match any directory named api at any depth.
In practice this is unlikely to cause issues, but it's a semantic change from the original behavior.
Minor: redundant pagePath/routePath normalization
The .replaceAll("\\", "/") added to pagePath/routePath in fileToAppRoute is redundant — consumers like app-dev-server.ts:61 already normalize backslashes via filePath.replace(/\\/g, "/"). It's also inconsistent with pages-router.ts which doesn't normalize filePath.
Node.js version requirement
fs.glob is experimental in Node 22 and stable from Node 23.5+. The package should declare "engines": { "node": ">=22" } at minimum to prevent confusing runtime errors for users on older Node versions.
| }); | ||
| const files: string[] = []; | ||
| const excludePrivate = (name: string) => name === "api" || name.startsWith("_"); | ||
| for await (const file of glob("**/*.{tsx,ts,jsx,js}", { cwd: pagesDir, exclude: excludePrivate })) { |
There was a problem hiding this comment.
Bug: excludePrivate over-excludes at nested depths.
The native glob() exclude callback receives each filesystem entry name during the walk — at every directory level, not just the root. This means:
name.startsWith("_")will exclude_-prefixed files/dirs inside subdirectories too (e.g.,pages/nested/_helper.tsxwould be excluded, but the oldignore: ["_*"]only matched at the pages root).name === "api"will match any directory namedapiat any depth (e.g.,pages/settings/api/), while the oldignore: ["api/**"]was root-relative.
I verified this experimentally: the old glob npm package's ignore: ["api/**", "_*"] patterns are root-relative, while the native exclude callback fires for every entry at every depth.
Suggested fix: collect all files and post-filter on the first path segment, matching the old behavior:
const allFiles: string[] = [];
for await (const file of glob("**/*.{tsx,ts,jsx,js}", { cwd: pagesDir })) {
allFiles.push(file);
}
const files = allFiles.filter((f) => {
const first = f.split(path.sep)[0];
return first !== "api" && !first.startsWith("_");
});Alternatively, keep exclude for api only (since nested api/ dirs are extremely unlikely) and post-filter for _ prefixes.
| import { glob } from "glob"; | ||
| import path from "node:path"; | ||
| import fs from "node:fs"; | ||
| import { glob } from "node:fs/promises"; |
There was a problem hiding this comment.
Since fs.glob is experimental in Node 22 (emits ExperimentalWarning) and only stable from Node 23.5+, consider adding "engines": { "node": ">=22" } to package.json. Without it, users on Node 20 or 18 will get a confusing glob is not a function runtime error.
| type === "page" | ||
| ? path.join(appDir, file).replaceAll("\\", "/") | ||
| : null, | ||
| routePath: | ||
| type === "route" | ||
| ? path.join(appDir, file).replaceAll("\\", "/") | ||
| : null, |
There was a problem hiding this comment.
This .replaceAll("\\", "/") normalization is redundant — consumers already normalize backslashes:
app-dev-server.ts:61:filePath.replace(/\\/g, "/")index.ts:590:r.filePath.replace(/\\/g, "/")
It's also inconsistent: pages-router.ts does not normalize filePath in its output. Consider removing this to keep both routers consistent and rely on the existing consumer-side normalization.
|
The review was submitted successfully. Here's a summary of what I found and posted: Review Summary for PR #52Verdict: Changes Requested 1. Behavioral regression in
|
- Fix excludePrivate over-exclusion: the native glob exclude callback
fires at every directory depth, not just the root. Replace with
post-filtering on the first path segment to match the old glob
package's root-relative ignore behavior.
- Remove redundant .replaceAll('\\', '/') normalization from
pagePath/routePath in app-router.ts — consumers already normalize
backslashes, and this was inconsistent with pages-router.ts.
- Add engines field (node >= 22) since fs.glob requires Node 22+.
- app-router: replace hasSlotSegment callback with ['**/@*'] pattern - pages-router: replace excludePrivate callback with ['api', '**/_*'] patterns - Simplify iteration: process glob results directly instead of collecting into intermediate arrays
|
Thank you! |
|
Hey @southpolesteve — heads up: I didn't see your commit before pushing mine, so I ended up fixing the same "exclude" issue independently. I went with glob pattern arrays ([exclude: ["api", "**/_*"]] instead of post-filtering. Your code got overwritten in the merge — just wanted to let you know so it doesn't catch you off guard. All tests pass. Thank you. |
|
All good! |
Node.js fs/promises.glob requires options.exclude to be a function. Passing an array throws TypeError on Node.js v23 and was never correct per the API spec (introduced in Node.js v22). PR cloudflare#52 migrated both routers from the glob npm package to the built-in but accidentally left array literals in the exclude option for both app-router.ts and pages-router.ts. Changes: - app-router.ts: `exclude: ["**/@*"]` → function that returns true when the entry's basename starts with "@" - pages-router.ts: `exclude: ["api", "**/_*"]` → function that returns true when the entry's basename is "api" or starts with "_" Behaviour is identical — parallel slot directories (@slot) and private pages (_app, _document, etc.) are still excluded from route scanning.
Node.js fs/promises.glob requires options.exclude to be a function. Passing an array throws TypeError on Node.js v23 and was never correct per the API spec (introduced in Node.js v22). PR cloudflare#52 migrated both routers from the glob npm package to the built-in but accidentally left array literals in the exclude option for both app-router.ts and pages-router.ts. Changes: - app-router.ts: `exclude: ["**/@*"]` → function that returns true when the entry's basename starts with "@" - pages-router.ts: `exclude: ["api", "**/_*"]` → function that returns true when the entry's basename is "api" or starts with "_" Behaviour is identical — parallel slot directories (@slot) and private pages (_app, _document, etc.) are still excluded from route scanning.
There was a problem hiding this comment.
Review
Good change. The core approach — using Symbol.for() + globalThis to share ALS instances across modules without circular imports — is sound. The guard placement, error messages, and test coverage are solid. I have one bug to flag and a couple of minor observations.
Bug: draftMode() is missing the cache scope guard
draftMode() reads state.headersContext.cookies — the same per-request cookie data that cookies() reads — but does not call throwIfInsideCacheScope(). If called inside a "use cache" or unstable_cache() scope, it would freeze the per-request draft mode state into the cached result. Worse, draftMode().enable() and .disable() mutate the cookie map and draftModeCookieHeader, which is a data integrity bug when done from a cached function.
Both headers() and cookies() have this guard; draftMode() should too:
// headers.ts, line 351
export async function draftMode(): Promise<DraftModeResult> {
throwIfInsideCacheScope("draftMode()"); // <-- missing
...This should also get a test.
Nit: isInsideCacheScope export is dead code
cache-runtime.ts:71 exports isInsideCacheScope(), but nothing in the codebase imports it. headers.ts re-implements the same check locally (via _isInsideUseCache()) using the globalThis Symbol lookup — which is the correct approach to avoid circular imports.
The export isn't harmful, but it's worth deciding whether it should exist. If it's intended as a public API for consumers, that's fine — just know it's untested via any import path. If it's leftover from an earlier design, consider removing it.
Observation: Guard ordering in headers() / cookies()
The cache scope check runs before the "no context" check:
export async function headers(): Promise<Headers> {
throwIfInsideCacheScope("headers()"); // <-- first
const state = _getState();
if (!state.headersContext) { throw ... } // <-- secondThis means if you call headers() inside a cache scope with no request context set up, you get the "cannot be called inside use cache" error rather than the "can only be called from a Server Component" error. That's the right priority — the cache scope error is more actionable and specific to the actual problem.
Test coverage
The 9 tests cover the matrix well (3 APIs x 2 scope types + 2 outside-scope + 1 regression). One gap: there's no test for nested scopes (e.g., unstable_cache inside "use cache" or vice versa). This is a minor edge case since either scope should independently trigger the guard, but it would be a good addition for completeness.
All existing tests pass (546/546). The approach is correct for dev/prod parity — the App Router prod/Cloudflare entries delegate to the compiled RSC handler which contains the same ALS nesting as dev, so the guards work in all environments.
Summary
Migrate
pagesRouter,apiRouter, andappRouterfrom the third-partyglobpackage to Node.js built-inglobfromnode:fs/promises(available since Node 22).Changes
pages-router.ts: Usefor awaitwith nativeglob(). Replaceignoreoption withexcludecallback for filteringapi/and_-prefixed files.app-router.ts: Usefor awaitwith nativeglob(). Replace post-filtering of@-prefixed slot segments withexcludecallback. NormalizepagePath/routePathoutput with.replaceAll("\\", "/")for cross-platform consistency.package.json: Removeglobdependency (no longer needed).Motivation
globnpm package is no longer needed since Node.js 22+ provides native glob support.globpackage returned paths with forward slashes on all platforms, which masked a bug where dynamic route segments (e.g.,[slug]) were incorrectly parsed due to premature path separator normalization. The native glob returns OS-native separators, and normalization is now done only on the final output metadata.Testing
All 50 routing tests pass (
pnpm test -- tests/routing.test.ts).