feat: define core types for virtual file system#13
Conversation
- Add FileSystemNode, File, Directory interfaces - Add FileSystem interface with CRUD operations - Add isFile and isDirectory type guards - Add tests for type guards Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use string (ISO 8601) instead of Date for serialization - Add SizeInBytes type alias for clarity - Add JSDoc for FileSystem interface Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use Date objects for createdAt/modifiedAt - Fix children type to FileSystemItem[] for type safety - Update type guards to accept FileSystemNode - Change rm() return type to void - Add JSDoc comments to all exports Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add comprehensive documentation for all VFS package features including: - Core data structures (File, Directory, FileSystemNode) - Maybe and Result monads - Helper functions (file, folder, type guards) - Path utilities - Core VFS API operations - Lock system - File system events - Shell module Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| * Virtual file system interface. | ||
| * This defines the API contract for implementing a virtual file system. | ||
| */ | ||
| export interface FileSystem { |
There was a problem hiding this comment.
The FileSystem interface has methods that return void or nullable types, but according to the documentation, these should return Result types for proper error handling. Consider aligning with the documented API that uses Result monads.
| root: Directory; | ||
| exists(path: string): boolean; | ||
| get(path: string): FileSystemItem | null; | ||
| mkdir(path: string): Directory; |
There was a problem hiding this comment.
The FileSystem interface specifies mkdir, touch, and rm operations but these are not yet implemented. Consider either removing these methods or documenting them as planned/coming soon in the interface.
| /** | ||
| * File type representing a file in the virtual file system. | ||
| */ | ||
| export type File = FileSystemNode & { |
There was a problem hiding this comment.
The size property is defined as SizeInBytes but there's no enforcement that it matches content.length. Consider either computing it dynamically or adding a runtime validation.
SummaryThis PR introduces core TypeScript types for a virtual file system (VFS) implementation, including file/directory types, a FileSystem interface, and type guard functions. Critical Issues
Recommendations
Positive Notes
|
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Code ReviewInline Commentspackages/vfs/src/types.ts:1 packages/vfs/src/types.ts:6-12 packages/vfs/src/types.ts:40-49
packages/vfs/src/types.ts:54-56 & 61-63 |
There was a problem hiding this comment.
Review Summary
Summary
The PR introduces core TypeScript types for a virtual file system with good structure and type safety. The implementation uses discriminated unions effectively and provides useful type guards.
Critical Issues
- Missing exports:
SizeInBytes,FileSystemNode, andFileSystemItemare not exported fromindex.ts. Users may need these base types to work with the VFS. - No path format specification: The
FileSysteminterface methods acceptpath: stringbut don't specify whether paths should be absolute, relative, or any particular format. - Error handling not defined: Methods like
mkdir(),rm(),write()don't indicate their error behavior - do they throw on failure?
Recommendations
- Add JSDoc documentation to
FileSysteminterface methods explaining path format expectations and error conditions. - Consider adding a
FileSystemErrortype or enum for consistent error handling across implementations. - The
sizeproperty inFilecould benefit from being optional or having validation - file size should be non-negative. - Consider adding a
readdir()method toFileSysteminterface for listing directory contents.
Positive Notes
- Well-structured type hierarchy with proper use of TypeScript features
- Good JSDoc documentation on most types
- Type guards are correctly implemented using type predicates
- Tests cover the type guards comprehensively
- Clean separation between
FileandDirectoryusing discriminated union pattern
Update documentation to reflect the actual code structure using intersection types for File and Directory extending FileSystemNode. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| @@ -0,0 +1,63 @@ | |||
| export type SizeInBytes = number; | |||
There was a problem hiding this comment.
The SizeInBytes type is just a type alias to number, which doesn't provide meaningful type safety. Consider using a branded type to prevent mixing with other numbers:
type SizeInBytes = number & { readonly brand: unique symbol };| mkdir(path: string): Directory; | ||
| touch(path: string, content?: string): File; | ||
| rm(path: string): void; | ||
| read(path: string): string | null; |
There was a problem hiding this comment.
The read method returns string | null, which cannot distinguish between "file not found" and "empty file content". Consider using the Result monad as documented in docs/README.md for consistent error handling across all methods.
| * Virtual file system interface. | ||
| * This defines the API contract for implementing a virtual file system. | ||
| */ | ||
| export interface FileSystem { |
There was a problem hiding this comment.
The documentation in docs/README.md shows a more comprehensive API using Result monads with discriminated union error types (e.g., { type: 'not_found'; path: string }). There's a mismatch between the documented API and this interface. Consider aligning them or updating the documentation.
| export type File = FileSystemNode & { | ||
| isDirectory: false; | ||
| content: string; | ||
| size: SizeInBytes; |
There was a problem hiding this comment.
There's no mechanism to ensure size stays in sync with content.length. Consider adding a helper function or making size a getter that computes the size dynamically:</```typescript
get size(): SizeInBytes {
return new TextEncoder().encode(this.content).length;
}
| @@ -0,0 +1,63 @@ | |||
| export type SizeInBytes = number; | |||
There was a problem hiding this comment.
Summary: Good foundation for the VFS core types with well-structured type hierarchy and type guards. The types are well-documented with JSDoc comments.
Critical Issues:
- API inconsistency between
types.tsand the documented API indocs/README.md- the interface uses nullable returns while docs show Result monads SizeInBytesis just a type alias providing no real type safety
Recommendations:
- Align the
FileSysteminterface with the documented Result-based error handling - Consider using branded types for
SizeInBytesto prevent arithmetic errors - Add size synchronization mechanism or make it a computed getter
- Add path validation types (the docs mention security against traversal attacks but no validation types exist)
Positive Notes:
- Clean type hierarchy with proper intersection types (
FileSystemNode & {...}) - Type guards
isFileandisDirectoryare correctly implemented - Good use of discriminated unions for
isDirectoryfield - Tests cover the type guards adequately
These have been extracted to an external @nesalia/fp package. Added note referencing the external package. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| * Virtual file system interface. | ||
| * This defines the API contract for implementing a virtual file system. | ||
| */ | ||
| export interface FileSystem { |
There was a problem hiding this comment.
The FileSystem interface defines a useful API contract, but there's a mismatch between what's declared here and what's documented in docs/README.md. The documentation shows methods like readFile, writeFile, mkdir, rm, copy, move, readDir, rename, lock, unlock, watch, etc., but the interface only has: exists, get, mkdir, touch, rm, read, write.
This is a critical issue because:
- The documentation describes an API that doesn't match the interface
- The actual implementation isn't provided in this PR (this is just type definitions)
Either the interface needs to be expanded to match the docs, or the docs need to be updated to reflect the actual interface.
| @@ -0,0 +1,500 @@ | |||
| # VFS Package Features | |||
There was a problem hiding this comment.
Issue: The documentation in docs/README.md references features that don't exist in the codebase yet (helper functions like file(), folder(), path utilities, lock system, file system events, shell module). The docs read as if all these features are already implemented, but only the core types exist.
Recommendation: Either:
- Remove the documentation for unimplemented features, or
- Add a prominent note at the top indicating this is a feature roadmap/backlog
| */ | ||
| export type Directory = FileSystemNode & { | ||
| isDirectory: true; | ||
| children: FileSystemItem[]; |
There was a problem hiding this comment.
The children type is FileSystemItem[] which creates a circular reference in the type definition. While this works in TypeScript, it could cause issues with certain tools (like some JSON schema generators). Consider if this needs special handling or documentation about the circular reference.
SummaryThis PR introduces core TypeScript types for a virtual file system ( Critical Issues
Recommendations
Positive Notes
|
- Add installation section - Remove redundant isDirectory from File type - Change children to Record for O(1) lookup - Remove path from FileSystemNode (derived from tree) - Fix API consistency (all operations return Result) - Fix timestamps to use Date consistently - Clarify Lock permissions with defaults - Add Planned Features section - Update Design Principles Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| ### FileSystemNode | ||
|
|
||
| ```typescript | ||
| type FileSystemNode = { |
There was a problem hiding this comment.
There's a significant mismatch between the code and the documentation. The documentation claims:
pathis NOT stored in FileSystemNode (derived from tree structure)childrenisRecord<string, FileSystemItem>for O(1) lookup- Helper functions
file()andfolder()exist
But the actual code has:
path: stringin FileSystemNode (line 8)children: FileSystemItem[](line 28)- No
file()orfolder()helper functions exported
| * Virtual file system interface. | ||
| * This defines the API contract for implementing a virtual file system. | ||
| */ | ||
| export interface FileSystem { |
There was a problem hiding this comment.
The FileSystem interface methods return different types inconsistently:
mkdir()returnsDirectory(not Result)touch()returnsFile(not Result)rm()returnsvoid(not Result)read()returnsstring | null(not Result)write()returnsFile(not Result)
But the documentation claims "All operations use the @deessejs/core Result monad for error handling". Either update the code to use Result types or update the documentation to reflect the actual API.
| export interface FileSystem { | ||
| root: Directory; | ||
| exists(path: string): boolean; | ||
| get(path: string): FileSystemItem | null; |
There was a problem hiding this comment.
The get() method returns FileSystemItem | null which requires null checking. Consider using Maybe<FileSystemItem> from @deessejs/core for consistency with the documented error handling pattern.
| */ | ||
| export type Directory = FileSystemNode & { | ||
| isDirectory: true; | ||
| children: FileSystemItem[]; |
There was a problem hiding this comment.
The children type is an array (FileSystemItem[]), but the documentation (line 72) states it should be Record<string, FileSystemItem> for O(1) lookup. Consider using Record for better performance.
| /** | ||
| * File type representing a file in the virtual file system. | ||
| */ | ||
| export type File = FileSystemNode & { |
There was a problem hiding this comment.
The isDirectory: false here is redundant since FileSystemNode already has isDirectory: boolean. However, this pattern allows the type guard to work properly, so it's a necessary TypeScript pattern for discrimination. Consider adding a comment explaining this for future maintainers.
| @@ -0,0 +1,58 @@ | |||
| import { describe, it, expect } from "vitest"; | |||
There was a problem hiding this comment.
Tests only cover type guards (isFile, isDirectory). No tests exist for the FileSystem interface since it's only an interface definition at this point. Consider adding tests when the implementation is added.
There was a problem hiding this comment.
Summary
This PR defines core TypeScript types for a virtual file system including File, Directory, FileSystemNode, FileSystemItem, and a FileSystem interface. It also includes type guards (isFile, isDirectory) and comprehensive documentation.
Critical Issues
-
Documentation Mismatch: The
docs/README.mdclaims features that don't exist in the code:pathis NOT stored (derived from tree) - but code haspath: stringchildrenisRecord<string, FileSystemItem>- but code hasFileSystemItem[]- Helper functions
file()andfolder()don't exist - FileSystem methods should return
Resultmonads but they return raw values
-
FileSystem Interface Inconsistency: Methods return different types inconsistently - some return raw values, some return null, contradicting the documented error handling pattern using Result monads.
Recommendations
-
Update the code to match the documentation (add helper functions, use Record for children, return Result types), OR
-
Update the documentation to match the current code (remove mentions of Result monads, helper functions, Record children)
-
Missing Test Coverage: No tests for the FileSystem interface (which is expected since it's only an interface at this stage)
-
API Design Decision: Consider whether
get()should returnMaybe<FileSystemItem>instead ofFileSystemItem | nullfor consistency with the monad-based error handling.
Positive Notes
- Good use of intersection types for File and Directory extending FileSystemNode
- Type guards are properly implemented with type predicates (
item is File) - JSDoc comments are present and helpful
- Test coverage for type guards is good with both positive and negative cases
- Separation of concerns is clear (types, exports, tests organized properly)
- Good use of the
SizeInBytestype alias for semantic clarity
Test Plan Verification
Since the build tools (turbo, pnpm) are not available in this environment, I couldn't run pnpm test, pnpm lint, or pnpm typecheck. Please ensure these pass locally before merging.
- Add Safety & Policies section: - Overwrite policy (fail/replace/merge) - Lock inheritance (bubble-up checking) - Circular reference prevention - Memory management - Add binary support (Uint8Array) to File type - Add mkdir recursive option - Add OverwriteMode to copy/move operations - Add ShellResult type with stdout/stderr/exitCode - Add parent_locked error type - Update Design Principles with Fail by Default and Safety First - Add more planned features (JSON export, async, streaming) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| ### FileSystemNode | ||
|
|
||
| ```typescript | ||
| type FileSystemNode = { |
There was a problem hiding this comment.
The documentation shows path as "derived from the tree structure, not stored in each node" but the actual type definition includes path: string as a required property. This is a mismatch that should be resolved.
| get(path: string): FileSystemItem | null; | ||
| mkdir(path: string): Directory; | ||
| touch(path: string, content?: string): File; | ||
| rm(path: string): void; |
There was a problem hiding this comment.
The rm method returns void which doesn't allow for error handling. Consider returning a type that can indicate success/failure (e.g., boolean or a Result type) for better error handling.
- Fix lock inheritance description (Inherited Protection) - Remove unimplemented redirection in Shell example - Clarify OverwriteMode merge behavior - Add readText/readBinary/read methods for binary support - Add atomic operations to planned features Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| get(path: string): FileSystemItem | null; | ||
| mkdir(path: string): Directory; | ||
| touch(path: string, content?: string): File; | ||
| rm(path: string): void; |
There was a problem hiding this comment.
Consider clarifying: Does this support recursive deletion for directories? What happens if the path doesn't exist - throw error or no-op?
Summary
File,Directory,FileSystemNode,FileSystemItemFileSysteminterface defining the VFS API contractisFile(),isDirectory()Test plan
pnpm testto verify all tests passpnpm lintto check code stylepnpm typecheckto verify TypeScript types🤖 Generated with Claude Code