Fix NodePath file URL conversion to follow the layer's platform flavor - #6914
Conversation
layerPosix and layerWin32 both built fromFileUrl and toFileUrl from node:url's host-dependent defaults, so off Windows layerWin32 returned POSIX file URLs while the rest of the layer used win32 semantics. Both fixed-flavor layers now pass the windows option through to node:url, and layer keeps the host default. Closes Effect-TS#6913 Signed-off-by: C. Spencer Beggs <spencer@beggs.codes>
🦋 Changeset detectedLatest commit: 15d6a3b The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
fileUrlOpsfactory: Replaced the top-levelfromFileUrl/toFileUrlconstants with afileUrlOps(windows)factory that threads the platform flavor through tonode:url'sfileURLToPath/pathToFileURL.layerPosix→fileUrlOps(false),layerWin32→fileUrlOps(true),layer→fileUrlOps(undefined).- New tests: Mirror the Deno path file-URL tests. The Windows test exercises the actual fix —
fileURLToPathwith{ windows: true }correctly returns backslash paths on a POSIX host. Both assertions usestrictEqual.
DeepSeek Pro (free via Pullfrog for OSS) (Kimi K2 not used — the program covers this model; add its provider key to run your pick) ｜ 𝕏
|
So, this is my fix for the issue. There are some failing tests in the Node layer, but those don't seem to be possibly related to my changes. I think they are test timeouts. Let me know what you think and if you would like any changes. |
|
Note on the one red check, so it does not cost a reviewer time.
It is not reachable from this diff. The test imports only from The change itself did get exercised: For what it is worth, the same base commit ( I can push an empty commit to re-roll it if that is useful, but I did not want to spend a CI run on cosmetics without asking. |
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
|
Does deno need updating too? |
|
@tim-smart Deno did it the right way (or this way). This just mirrors how Deno works into the |
2 participants
Closes #6913.
NodePathbuiltfromFileUrl/toFileUrlonce fromnode:url's host-dependent defaults and spread that same pair into all three layers, solayerWin32disagreed with itself off Windows:join/dirname/sepwere win32 while URL conversion stayed POSIX.layerPosixhad the mirror defect on a Windows host.node:urlhas exposed the flavor as an option since v22.1.0, so this threads it through rather than reimplementing the conversion:fileUrlOps(false)forlayerPosix,fileUrlOps(true)forlayerWin32, andfileUrlOps(undefined)forlayer, which resolves to the host default and leaves the host layer's behavior unchanged.The new test mirrors the existing
packages/platform-deno/test/DenoPath.test.tsfile-URL tests, which already assert this forDenoPath.layerPosix/layerWin32. Before the change the Windows case failed on darwin withexpected '/C:/Users/me/file.txt' to equal 'C:\Users\me\file.txt'.Two things worth a maintainer's judgement:
packages/platform-node-shared/package.jsondeclaresengines.node: ">=18.0.0", and on Node < 22.1 the extra options argument is silently ignored, so both fixed-flavor layers fall back to host behavior there. That is no worse than today, but it is not fixed either. Since Node 18 and 20 are both EOL, bumpingenginesseems more honest than vendoring a parallel copy of Node's conversion logic, but I did not want to make that call in this PR.{ windows: false }and the host agree, and this repo's CI is ubuntu-only (runs-on: ubuntu-latestacross the whole shard/runtime matrix). So nothing here exercises thelayerPosix-on-Windows case, and I have no Windows host to check it on either; that half of the fix is reasoned fromnode:url's platform branch rather than measured. The win32 half is what actually demonstrates the change, and it runs everywhere.Validation run locally on Node 26.5.0 / darwin:
pnpm lint-fix,pnpm check,vitest run --project @effect/platform-node-shared(106 passed),vitest run --project @effect/platform-node(277 passed). One run of the shared suite hit a pre-existing flake inNodeStream.test.ts(PassThrough.onEnd,src/NodeStream.ts:354) that did not reproduce across three subsequent runs and is unrelated to this change.Signed-off-by: C. Spencer Beggs spencer@beggs.codes