Skip to content

refactor: migrate path handling from node:path to pathslash#2502

Open
shulaoda wants to merge 17 commits into
cloudflare:mainfrom
shulaoda:pathslash-migration
Open

refactor: migrate path handling from node:path to pathslash#2502
shulaoda wants to merge 17 commits into
cloudflare:mainfrom
shulaoda:pathslash-migration

Conversation

@shulaoda

@shulaoda shulaoda commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

What

Migrates all of packages/vinext/src (~100 files) from node:path to pathslash, in eight batch commits:

  1. refactor: CLI cluster (typegen, check, cli, init, init-cloudflare, utils/project) + the pathslash dependency via pnpm catalog
  2. refactor(routing): app-route-graph, pages-router, file-matcher, app-router + the vi.mock rewrite
  3. refactor(plugins): 13 plugin files
  4. refactor(server): 11 server files
  5. refactor(build): 15 build files
  6. refactor(core): entries, config, and shared utils (18 files)
  7. refactor(plugin): index.ts (the Vite plugin heart, ~25 normalization sites)
  8. refactor: endgame — test-suite sweep to toSlash, stragglers, no-restricted-imports lint guard

Follow-up commits address CI findings (bundled-dependency output layout, Windows-only fixture gating) and track main: the prerender path-discovery feature (#2481) landed mid-review and is migrated in refactor: migrate upstream prerender path discovery to pathslash.

Why

The forward-slash invariant used to live in JSDoc prose ("root must be forward-slash — callers normalize it at their entry…") and had to be restated and hand-maintained at every hop, which produced a five-week, 50+-commit whack-a-mole campaign — and stragglers still kept surfacing. pathslash turns that convention into an API guarantee: it delegates every operation to the real node:path (drive letters, per-drive cwd, case-insensitive relative keep native semantics) and only converts Windows output separators to /. Once a module imports it, every join/resolve/relative/dirname result is canonical forward-slash by construction, so the per-function contracts, the path.posix.* rewrites, and the normalizePathSeparators(...) wrappers all disappear from migrated code. Notably, pathslash.relative bakes in the rule #2308 learned the hard way: relativize natively, then normalize the output.

How

  • pathslash is a devDependency, bundled into dist via the same deps.alwaysBundle mechanism as am-i-vibing — it is a ~90-line zero-dependency wrapper, so end users never download it (single shared module in the emitted graph, nothing added to the install footprint).
  • Inlined dependencies are emitted under dist/deps/... instead of the default dist/node_modules/... mirror (outputOptions.entryFileNames/chunkFileNames rename). build/standalone.ts prunes every path containing a node_modules segment when assembling standalone output, so an inlined dep that the server statically imports (pathslash via prod-server) would otherwise go missing at standalone-server boot — a latent trap am-i-vibing/process-ancestry had only dodged because nothing in the server graph imports them.
  • import path from "node:path"import path from "pathslash"; path.posix.*path.*; normalizePathSeparators(path.X(...)) → bare path.X(...).
  • toSlash marks the boundaries where external-origin strings enter the canonical space: process.cwd(), config.root, fileURLToPath, require.resolve, realpathSync.native, node's glob output, and Rolldown/Vite ids reported with native separators (resolveId importer-joined virtual ids, watchChange). Everything in between needs no normalization at all.
  • The platform gate is the contract everywhere — Windows-style paths do not occur on POSIX at runtime, so source never carries unconditional .replaceAll("\\", "/") fallbacks. Tests that feed Windows-shaped fixtures (chunk-classification ids, V8 stack frames) are gated with it.runIf(process.platform === "win32") with their assertions left unconditional; see tests/build-optimization.test.ts, tests/dev-stack-sourcemap.test.ts.
  • All "must be forward-slash" JSDoc contracts deleted outright — the guarantee lives in the import now.
  • normalizePathSeparators removed from utils/path.ts (kept: isWindows, stripViteModuleQuery, stripJsExtension); the test suite's 39 wrap sites moved to toSlash from pathslash.
  • New lint guard: no-restricted-imports bans node:path in packages/vinext/src — the durable replacement for prose contracts. The convention (pathslash in source, node:path inputs + toSlash-wrapped expectations in tests, when to use toSlash, how to opt a file out) is documented in AGENTS.md under Code Style.
  • Deliberate holdouts, each with a reason in place: build/standalone.ts (native-space tree copier, inline lint-disable), server/app-ssr-entry.ts (dynamic import("node:path") in a runtime React fallback where a package dependency would change bundling), URL-space backslash defenses (prod-server, image-optimization, index.ts redirect sanitizing), init-cloudflare.ts generated-config literals, and the tsconfig-extends classifier that must accept user-authored backslash paths.
  • tests/app-route-graph.test.ts's Windows-simulation mock now mocks pathslash itself (substituting the cross-platform win32 flavor), so POSIX CI exercises the real conversion instead of a no-op helper.
  • Windows bug fixes that fell out of the migration: root-layout tree position mis-counted with a native appDir (computeLayoutTreePositions), chokidar watcher add/unlink route invalidation never matching on Windows (index.ts), and the fonts cached-CSS rewrite that silently no-opped on Windows.

Testing

  • vp check (format + lint + types) green throughout; every batch committed only after its verification gate.
  • Windows machine (real backslash environment): the migration fixed ~50 previously-failing Windows unit tests (routing/app-route-graph graphs, static-file-cache, tsconfig aliases, trailing-slash, asset-prefix, dynamic-requests fixtures…). Test expectations comparing router/build outputs moved to canonical form via toSlash wraps — inputs deliberately stay native node:path so Windows runs keep exercising the conversion.
  • Remaining known-failing tests are pre-existing and unrelated to separators: NTFS-invalid fixture filenames (:/*), the pinned performance-benchmarks environment, and a handful of load-flaky dev-server tests (verified green in isolation).
  • Attribution method: every suspicious failure was compared against a clean HEAD baseline worktree on the same machine before being treated as a regression.
  • Two adversarial multi-agent reviews (batch 1 and the final whole-branch pass) over call-site equivalence, cross-file coherence, Windows semantics, and test masking.

@pkg-pr-new

pkg-pr-new Bot commented Jul 3, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2502
npm i https://pkg.pr.new/create-vinext-app@2502
npm i https://pkg.pr.new/vinext@2502

commit: d321f29

@shulaoda shulaoda marked this pull request as draft July 3, 2026 10:51
@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Performance benchmarks

Compared d321f29 against base fd1cc3d using alternating same-runner rounds. Next.js was unchanged and skipped.

0 improved · 0 regressed · 6 within ±1.5%

Scenario Framework Baseline Current Change
Client bundle size (gzip) vinext 126.3 KB 126.3 KB ⚫ -0.0%
Client entry size (gzip) vinext 120.3 KB 120.3 KB ⚫ -0.0%
Dev server cold start vinext 2.68 s 2.68 s ⚫ +0.1%
Production build time vinext 3.16 s 3.17 s ⚫ +0.6%
RSC entry closure size (gzip) vinext 97.5 KB 98.0 KB ⚫ +0.4%
Server bundle size (gzip) vinext 164.0 KB 164.4 KB ⚫ +0.3%

View detailed results and traces

🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head

@shulaoda shulaoda marked this pull request as ready for review July 3, 2026 11:25
@shulaoda shulaoda marked this pull request as draft July 3, 2026 11:33
@shulaoda shulaoda marked this pull request as ready for review July 3, 2026 12:29
@shulaoda shulaoda force-pushed the pathslash-migration branch from 6aa72cf to b8670c7 Compare July 5, 2026 01:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant