Dir.{dispose,asyncDispose} and Dirent.parentPath
#1225
Conversation
streamich
changed the title
Dir.{disposable,asyncDisposable and Dirent.parentPathDir.{disposable,asyncDisposable} and Dirent.parentPath
streamich
changed the title
Dir.{disposable,asyncDisposable} and Dirent.parentPathDir.{dispose,asyncDispose} and Dirent.parentPath
There was a problem hiding this comment.
Pull request overview
This PR adds support for the Dir.disposable and Dir.asyncDisposable Symbol methods, and implements the Dirent.parentPath property as specified in Node.js. The Dirent.path property is deprecated in favor of parentPath.
Changes:
- Added
Symbol.disposeandSymbol.asyncDisposemethods to theDirclass - Added
parentPathproperty toIDirentinterface and implementations - Deprecated the
pathproperty in favor ofparentPath
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Updated exclude pattern to match nested test directories |
| src/node/types/misc.ts | Added Disposable/AsyncDisposable interfaces to IDir, added parentPath property to IDirent |
| src/node/Dirent.ts | Reordered properties to make parentPath primary, deprecated path property |
| src/node/Dir.ts | Implemented Symbol.dispose and Symbol.asyncDispose methods |
| src/core/Link.ts | Fixed getParentPath to return root separator for empty parent |
| src/node/tests/Dirent.test.ts | Added comprehensive tests for parentPath property |
| src/node/tests/Dir.test.ts | Added tests for disposable symbols |
| src/node/tests/promises.test.ts | Added tests for promises.opendir |
| src/node/tests/volume/readdirSync.test.ts | Updated tests to use parentPath and fixed property order |
| src/fsa-to-node/FsaNodeDirent.ts | Added parentPath to constructor and deprecated path property |
| src/fsa-to-node/FsaNodeFs.ts | Updated to pass parentPath to FsaNodeDirent, replaced hardcoded '/' with constant |
| src/fsa-to-node/tests/FsaNodeDirent.test.ts | Added comprehensive tests for FsaNodeDirent.parentPath |
| files: T; | ||
| type: T; | ||
| } | ||
|
|
There was a problem hiding this comment.
The Disposable and AsyncDisposable types are being used without import statements. These are built-in TypeScript types available in ES2022/ES2024, but the tsconfig.json specifies target "es2020" and lib ["ES2020", "dom"]. This may cause type errors if these types are not available in the TypeScript standard library for ES2020. Consider adding an explicit import or updating the lib to include ES2022/ES2024 disposables support, or defining these interfaces locally.
| declare global { | |
| interface SymbolConstructor { | |
| readonly dispose: symbol; | |
| readonly asyncDispose: symbol; | |
| } | |
| } | |
| interface Disposable { | |
| [Symbol.dispose](): void; | |
| } | |
| interface AsyncDisposable { | |
| [Symbol.asyncDispose](): PromiseLike<void> | void; | |
| } |
| const all = vol.readdirSync('/x', { withFileTypes: true }); | ||
| const mapped = all.map(dirent => { | ||
| return { ...dirent }; | ||
| return { ...(dirent as any) }; |
There was a problem hiding this comment.
Using as any bypasses TypeScript's type safety. Consider creating a proper type for the expected shape of the spread object instead of using type assertion. This pattern appears multiple times in the test file and reduces the value of type checking.
| }); | ||
|
|
||
| describe('disposable', () => { | ||
| it('should be closed for disposable', done => { |
There was a problem hiding this comment.
The test name "should be closed for disposable" is unclear. Consider renaming to "should close directory when Symbol.dispose is called" to better describe what is being tested.
| }); | ||
| }); | ||
|
|
||
| it('should be closed for async disposable', done => { |
There was a problem hiding this comment.
The test name "should be closed for async disposable" is unclear. Consider renaming to "should close directory when Symbol.asyncDispose is called" to better describe what is being tested.
| it('should be closed for async disposable', done => { | |
| it('should close directory when Symbol.asyncDispose is called', done => { |
| it('Reject when directory is a file', () => { | ||
| return expect(promises.opendir('/foo/bar')).rejects.toThrow(/ENOTDIR/); | ||
| }); |
There was a problem hiding this comment.
Test naming inconsistency: This test uses title case ("Reject when...") while the previous test uses sentence case ("Open an existing directory"). Consider using consistent naming convention, preferably lowercase for the first word to match Jest conventions (e.g., "rejects when directory is a file").
|
🎉 This PR is included in version 4.52.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
No description provided.