-
Notifications
You must be signed in to change notification settings - Fork 5k
node:fs: implement mkdtempDisposable / mkdtempDisposableSync #31019
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
sam-shridhar1950f
wants to merge
1
commit into
oven-sh:main
from
sam-shridhar1950f:node-fs-mkdtemp-disposable
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,7 @@ import fs, { | |
| lstatSync, | ||
| mkdirSync, | ||
| mkdtemp, | ||
| mkdtempDisposableSync, | ||
| mkdtempSync, | ||
| openAsBlob, | ||
| openSync, | ||
|
|
@@ -4043,3 +4044,101 @@ describe.skipIf(isWindows)("readFileSync on a FIFO larger than the stat size", ( | |
| expect(exitCode).toBe(0); | ||
| }); | ||
| }); | ||
|
|
||
| describe("mkdtempDisposableSync", () => { | ||
| it("creates a directory and removes it via Symbol.dispose", () => { | ||
| const result = mkdtempDisposableSync(join(tmpdirSync(), "disposable-")); | ||
| expect(typeof result.path).toBe("string"); | ||
| expect(existsSync(result.path)).toBe(true); | ||
| result[Symbol.dispose](); | ||
| expect(existsSync(result.path)).toBe(false); | ||
| }); | ||
|
|
||
| it("calling dispose twice is safe", () => { | ||
| const result = mkdtempDisposableSync(join(tmpdirSync(), "disposable-")); | ||
| result[Symbol.dispose](); | ||
| result[Symbol.dispose](); | ||
| expect(existsSync(result.path)).toBe(false); | ||
| }); | ||
|
|
||
| it("remove() works as a manual alternative to dispose", () => { | ||
| const result = mkdtempDisposableSync(join(tmpdirSync(), "disposable-")); | ||
| result.remove(); | ||
| expect(existsSync(result.path)).toBe(false); | ||
| }); | ||
|
|
||
| it("works with `using` syntax", () => { | ||
| let savedPath: string; | ||
| { | ||
| using temp = mkdtempDisposableSync(join(tmpdirSync(), "disposable-")); | ||
| savedPath = temp.path; | ||
| expect(existsSync(temp.path)).toBe(true); | ||
| } | ||
| expect(existsSync(savedPath!)).toBe(false); | ||
| }); | ||
|
|
||
| it("removes a non-empty directory recursively", () => { | ||
| const result = mkdtempDisposableSync(join(tmpdirSync(), "disposable-")); | ||
| writeFileSync(join(result.path, "nested.txt"), "hello"); | ||
| mkdirSync(join(result.path, "sub")); | ||
| writeFileSync(join(result.path, "sub", "deep.txt"), "world"); | ||
| result[Symbol.dispose](); | ||
| expect(existsSync(result.path)).toBe(false); | ||
| }); | ||
|
|
||
| it("survives process.chdir between create and dispose", () => { | ||
| const originalCwd = process.cwd(); | ||
| const base = tmpdirSync(); | ||
| process.chdir(base); | ||
| try { | ||
| const result = mkdtempDisposableSync("./disposable-"); | ||
| const absolute = path.resolve(base, result.path); | ||
| expect(existsSync(absolute)).toBe(true); | ||
| process.chdir(os.tmpdir()); | ||
| result[Symbol.dispose](); | ||
| expect(existsSync(absolute)).toBe(false); | ||
| } finally { | ||
| process.chdir(originalCwd); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| describe("fs.promises.mkdtempDisposable", () => { | ||
| it("creates a directory and removes it via Symbol.asyncDispose", async () => { | ||
| const result = await _promises.mkdtempDisposable(join(tmpdirSync(), "disposable-")); | ||
| expect(typeof result.path).toBe("string"); | ||
| expect(existsSync(result.path)).toBe(true); | ||
| await result[Symbol.asyncDispose](); | ||
| expect(existsSync(result.path)).toBe(false); | ||
| }); | ||
|
|
||
| it("works with `await using` syntax", async () => { | ||
| let savedPath: string; | ||
| { | ||
| await using temp = await _promises.mkdtempDisposable(join(tmpdirSync(), "disposable-")); | ||
| savedPath = temp.path; | ||
| expect(existsSync(temp.path)).toBe(true); | ||
| } | ||
| expect(existsSync(savedPath!)).toBe(false); | ||
| }); | ||
|
|
||
| it("calling asyncDispose twice is safe", async () => { | ||
| const result = await _promises.mkdtempDisposable(join(tmpdirSync(), "disposable-")); | ||
| await result[Symbol.asyncDispose](); | ||
| await result[Symbol.asyncDispose](); | ||
| expect(existsSync(result.path)).toBe(false); | ||
| }); | ||
|
|
||
| it("remove() returns a Promise that resolves", async () => { | ||
| const result = await _promises.mkdtempDisposable(join(tmpdirSync(), "disposable-")); | ||
| await result.remove(); | ||
| expect(existsSync(result.path)).toBe(false); | ||
| }); | ||
|
|
||
| it("is also reachable via the node:fs `promises` namespace", async () => { | ||
| const result = await promises.mkdtempDisposable(join(tmpdirSync(), "disposable-")); | ||
| expect(existsSync(result.path)).toBe(true); | ||
| await result[Symbol.asyncDispose](); | ||
| expect(existsSync(result.path)).toBe(false); | ||
| }); | ||
| }); | ||
|
Comment on lines
+4106
to
+4144
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win Add a The sync suite verifies cwd drift safety, but the async suite doesn’t. Add one equivalent async case to lock in the captured-cwd cleanup behavior. 🤖 Prompt for AI Agents |
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Replace
tmpdirSync()withtempDir(...)in these new tests.These new cases create temp roots with
tmpdirSync(), which conflicts with the test harness rule and loses disposable cleanup ergonomics.Suggested pattern
As per coding guidelines: "Use
tempDirfrom 'harness' to create temporary directories - do not usetmpdirSyncorfs.mkdtempSync."Also applies to: 4046-4046, 4053-4053, 4061-4061, 4069-4069, 4079-4079, 4096-4096, 4106-4106, 4114-4114, 4121-4121
🤖 Prompt for AI Agents