Fix Deno filesystem error context - #7086
Conversation
🦋 Changeset detectedLatest commit: 6dfba01 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
This PR fixes error-context loss in the Deno FileSystem implementation and normalizes an error mapping for @std/fs copy.
.changeset/deno-write-copy-errors.md— adds a patch changeset for@effect/platform-deno.packages/platform-deno/src/DenoFileSystem.ts— threadsmethodandpathOrDescriptorthroughFileImpl.writeChunk/writeAllChunk/writeAllso that high-levelwriteFilecalls report the original method and path even when they delegate to the file-handle writer.packages/platform-deno/src/internal/error.ts— mapsDeno.errors.AlreadyExistsbynameso that errors produced without an errno-stylecodeare tagged correctly.packages/platform-deno/test/DenoFileSystem.test.ts— adds regression tests for thewriteFileerror context and thecopyAlreadyExistsmapping.packages/platform-deno/test/internal/error.test.ts— updates theAlreadyExistsunit test to cover the name-only case.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
| })) | ||
| }) | ||
|
|
||
| it.effect("reports delegated write errors as writeFile errors", () => |
There was a problem hiding this comment.
Add these tests to the shared suite instead.
| return Effect.acquireUseRelease( | ||
| tryPromise("writeFile", path, () => Deno.open(path, openOptions(flag, options?.mode))), | ||
| (file) => new FileImpl(file, flag.startsWith("a")).writeAll(data), | ||
| (file) => new FileImpl(file, flag.startsWith("a")).writeAll(data, "writeFile", path), |
There was a problem hiding this comment.
I think just map the system error reason instead of notifying FileImpl
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This re-review covers the follow-up commit that addressed prior review feedback.
packages/platform-deno/src/DenoFileSystem.ts— replaced theFileImplparameter-threading approach with a singleEffect.mapErrorinwriteFile, which rewrites the system error'smethodandpathOrDescriptorat the call site. This keepsFileImpl.writeAll/writeChunk/writeAllChunkunchanged and avoids leaking high-level metadata into the internal file writer.packages/effect/test/FileSystem.test-utils.ts— added thewriteFileerror-context andcopyAlreadyExistsregression tests to the shared cross-platform suite.packages/platform-deno/test/DenoFileSystem.test.ts— simplified to just invoke the sharedtestLayer, removing the now-duplicate Deno-specific tests.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
| yield* fs.writeFileString(path, "data", { flag: "r" }).pipe(Effect.flip) | ||
| const error = yield* fs.writeFileString(path, "data", { flag: "r" }).pipe(Effect.flip) | ||
|
|
||
| assert(error.reason instanceof PlatformError.SystemError) |
There was a problem hiding this comment.
Check the tag instead of using instanceof
| (file) => new FileImpl(file, flag.startsWith("a")).writeAll(data), | ||
| (file) => | ||
| new FileImpl(file, flag.startsWith("a")).writeAll(data).pipe( | ||
| Effect.mapError((error) => |
There was a problem hiding this comment.
Don't use instanceof
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This re-review covers the follow-up commit that switched the new shared filesystem assertions from instanceof checks to _tag checks.
packages/effect/test/FileSystem.test-utils.ts— replacedPlatformError.SystemError/PlatformErrorimports andinstanceofassertions with_tag !== "BadArgument"/_tag === "AlreadyExists"checks in the newwriteFileerror-context andcopyAlreadyExiststests.packages/platform-deno/src/DenoFileSystem.ts— aligned themapErrorguard fromerror.reason instanceof PlatformError.SystemErrortoerror.reason._tag !== "BadArgument", matching the shared test style.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
1 participant
Summary
writeFilemethod and path context when high-level Deno writes delegate toFileImplDeno.errors.AlreadyExistsfailures produced by@std/fscopy@effect/platform-denoRoot cause
Non-native
writeFileflag handling reusedFileImpl.writeAll, which labeled failures aswriteAllwithout the original path. Separately, the copy helper constructsDeno.errors.AlreadyExistswithout an errno-stylecode, while the adapter only recognizedEEXIST.Validation
deno task test --run --project @effect/platform-deno(205 passed, 11 skipped)pnpm --filter @effect/platform-deno checkdprintandoxlintchecks on the changed filesCloses EFF-527