fs: runtime deprecate closing fs.Dir on garbage collection - #58850
fs: runtime deprecate closing fs.Dir on garbage collection#58850LiviaMedeiros wants to merge 1 commit into
Conversation
Sorry, something went wrong.
Codecov Report❌ Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.
@@ Coverage Diff @@
## main #58850 +/- ##
==========================================
- Coverage 88.28% 88.26% -0.02%
==========================================
Files 702 702
Lines 206760 206773 +13
Branches 39776 39777 +1
==========================================
- Hits 182531 182508 -23
- Misses 16235 16269 +34
- Partials 7994 7996 +2
... and 36 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Sorry, something went wrong.
| import { opendir } from 'node:fs/promises'; | ||
|
|
||
| { | ||
| await using dir = await opendir('/async/disposable/directory'); | ||
| } // Closed by dir[Symbol.asyncDispose]() | ||
|
|
||
| { | ||
| using dir = await opendir('/sync/disposable/directory'); | ||
| } // Closed by dir[Symbol.dispose]() | ||
|
|
||
| { | ||
| let dir; | ||
| try { | ||
| dir = await opendir('/legacy/closeable/directory'); | ||
| } finally { | ||
| await dir?.close(); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this is lacking the (IMO) most prominent use-case for opendir: iterating on it. Can you add a test that iterates on a Dir and validates that it does not trigger any warning?
Sorry, something went wrong.
There was a problem hiding this comment.
Can you add the iteration example in this snippet as well please?
Sorry, something went wrong.
There was a problem hiding this comment.
Sure, added.
Sorry, something went wrong.
aduh95
left a comment
There was a problem hiding this comment.
LGTM. We should probably land this as doc-only first, if you can open another PR that lands first, that'd be great, otherwise I can do it for you
Sorry, something went wrong.
|
Sure, i'll make PR with doc-only version of this (or rather, formalizing already existing runtime warning as deprecation). |
Sorry, something went wrong.
Sorry, something went wrong.
|
@nodejs/tsc do we want this in v25.x? If so, we'd need at least one more approval |
Sorry, something went wrong.
|
This pull request has been marked as stale due to 90 days of inactivity. |
Sorry, something went wrong.
The DEP0137 is going to be End-of-Life in the next major release.
I think we should align closing fs.Dir on GC and deprecate it as well. Now that explicit resource management is available, closing dirs properly should become easier for userland.
Given that it already spams Warnings in stderr, it would make sense to make this deprecation runtime right away.
cc @nodejs/fs @nodejs/tsc