-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat: Implement memfs for file system management #2281
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
base: main
Are you sure you want to change the base?
Conversation
@irfanfaraaz is attempting to deploy a commit to the Onlook Team on Vercel. A member of the Team first needs to authorize it. |
This is awesome! What made you choose memfs out of the other solutions? Just curious if there were tradeoffs you considered. |
When I researched VFS libraries, memfs was the most recommended because of it's focus on in-memory use and works as a lightweight, near drop-in replacement for Node's fs. Other libraries are more focused on disk storage |
That makes sense. Seems consistent with a deep research run since I haven't used these libraries Is all the behavior the same for the code tab, AI and assets management or are you seeing any differences there? |
Yes the behavior is the same. I used Gemini for testing since i ran out of Anthropic credits, so can’t fully comment on performance. There’s a bit of inconsistency in tool calling, but it was present both before and after my changes. |
} | ||
|
||
// Path utilities | ||
normalizePath(path: string): string { |
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.
It’d be great to double-check these utility functions like normalizePath, dirname, and basename to ensure they align with our existing ones in the files utility. Currently, these functions are returning different paths compared to the existing ones.
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.
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.
It looks good overall! But could you kindly rebase with main and verify if Asset Management still works with this implementation? |
Thanks for pointing it out. Updated the code to use existing utility functions and added separate logic for memfs, since memfs requires absolute paths. |
Could you share more details about the errors you're encountering? |
@irfanfaraaz could you try pulling from latest and run |
Oh finally it's working.. guess morphllm was down above screenshot is from vfs branch |
Nice! Glad it's working. We're using relace for the prod version but interesting that morph went down |
@@ -439,20 +458,23 @@ export class VirtualFileSystem implements VirtualFileSystemInterface { | |||
for (const sandboxPath of allFiles) { | |||
try { | |||
const vfsPath = this.toVFSPath(sandboxPath); | |||
// Try to read as text first | |||
const textContent = this.volume.readFileSync(vfsPath, { |
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.
The issue is that readFileSync with encoding: 'utf8' will attempt to decode binary files as UTF-8 text, which often succeeds but produces garbled/corrupted text instead of throwing an error. This means the catch block never executes for binary files.
const vfsPath = this.toVFSPath(sandboxPath); | ||
|
||
// Determine if file is binary based on extension | ||
const isBinary = this.isBinaryFile(sandboxPath); |
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.
So I wrote an utility function to check
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.
Oh, I wasn’t aware of that. Got to learn something. Thanks for the changes, mate! @spartan-vutrannguyen
ctime: Date; | ||
} | ||
|
||
export interface VirtualFileSystemInterface extends FileOperations { |
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.
Not sure I understand why we need to re-implement this interface instead using the direct fs api from memfs
https://github.com/streamich/memfs/blob/master/docs/node/usage.md
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.
Here’s what I’m doing and why:
Using Volume from memfs instead of (fs,vol):
I'm using Volume so each project/sandbox has its own isolated file system.
Wrapper Methods:
The wrappers handle path normalization (so callers can use relative paths for memfs) with the underlying this.volume instance
Persistence (new commit):
I switched to memfs’s toJSON()/fromJSON() for saving/restoring the file system.
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.
The goal I was hoping to achieve was to potentially reduce the amount of code we have implementing a file system. To potentially have a better way to mirror the files system from the sandbox without implementing file system interfaces. Looks like this would not be feasible even with using memfs?
Seems like it's essentially the same amount of work and complexity with just a different persistence layer. I was hoping we would remove this class entirely and use memfs' interface directly.
Description
Migrated the sandbox file system from custom file/content maps to a proper Virtual File System (VFS) implementation using memfs. This change significantly improves performance, reduces complexity, and provides enhanced file system capabilities while maintaining 100% backward compatibility.
Replaced
FileSyncManager
withVFSSyncManager
andvirtual-fs.ts
Related Issues
fixes #2280
Type of Change
Testing
VirtualFileSystem
: 22 tests covering core file system functionalityVFSSyncManager
: 21 tests ensuring compatibility and new featuresUpdated existing tests: All 82 sandbox tests passing
Screenshots (if applicable)
Additional Notes
Files added:
virtual-fs.ts
- Core VFS implementationvfs-sync-manager.ts
- Drop-in replacement for FileSyncManagerImportant
Replaces
FileSyncManager
withVFSSyncManager
usingmemfs
for improved file system management, adding new VFS implementation and extensive testing.FileSyncManager
withVFSSyncManager
insandbox/index.ts
.virtual-fs.ts
for core VFS implementation.vfs-sync-manager.ts
as a drop-in replacement forFileSyncManager
.VirtualFileSystem
covering core functionality.VFSSyncManager
ensuring compatibility and new features.memfs
topackage.json
for VFS support.This description was created by
for 07692dd. You can customize this summary. It will automatically update as commits are pushed.