Conversation
|
Review requested:
|
|
The
notable-change
Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section. |
|
Nice! This is a great addition. Since it's such a large PR, this will take me some time to review. Will try to tackle it over the next week. |
| */ | ||
| existsSync(path) { | ||
| // Prepend prefix to path for VFS lookup | ||
| const fullPath = this.#prefix + (StringPrototypeStartsWith(path, '/') ? path : '/' + path); |
| validateObject(files, 'options.files'); | ||
| } | ||
|
|
||
| const { VirtualFileSystem } = require('internal/vfs/virtual_fs'); |
There was a problem hiding this comment.
Shouldn't we import this at the top level / lazy load it at the top level?
| ArrayPrototypePush(this.#mocks, { | ||
| __proto__: null, | ||
| ctx, | ||
| restore: restoreFS, |
There was a problem hiding this comment.
| restore: restoreFS, | |
| restore: ctx.restore, |
nit
lib/internal/vfs/entries.js
Outdated
| * @param {object} [options] Optional configuration | ||
| */ | ||
| addFile(name, content, options) { | ||
| const path = this._directory.path + '/' + name; |
lib/internal/vfs/virtual_fs.js
Outdated
| let entry = current.getEntry(segment); | ||
| if (!entry) { | ||
| // Auto-create parent directory | ||
| const dirPath = '/' + segments.slice(0, i + 1).join('/'); |
lib/internal/vfs/virtual_fs.js
Outdated
| let entry = current.getEntry(segment); | ||
| if (!entry) { | ||
| // Auto-create parent directory | ||
| const parentPath = '/' + segments.slice(0, i + 1).join('/'); |
lib/internal/vfs/virtual_fs.js
Outdated
| } | ||
| } | ||
| callback(null, content); | ||
| }).catch((err) => { |
There was a problem hiding this comment.
| }).catch((err) => { | |
| }, (err) => { |
lib/internal/vfs/virtual_fs.js
Outdated
| const bytesToRead = Math.min(length, available); | ||
| content.copy(buffer, offset, readPos, readPos + bytesToRead); |
lib/internal/vfs/virtual_fs.js
Outdated
| } | ||
|
|
||
| callback(null, bytesToRead, buffer); | ||
| }).catch((err) => { |
There was a problem hiding this comment.
| }).catch((err) => { | |
| }, (err) => { |
|
Left an initial review, but like @Ethan-Arrowood said, it'll take time for a more in depth look |
|
It's nice to see some momentum in this area, though from a first glance it seems the design has largely overlooked the feedback from real world use cases collected 4 years ago: https://github.com/nodejs/single-executable/blob/main/docs/virtual-file-system-requirements.md - I think it's worth checking that the API satisfies the constraints that users of this feature have provided, to not waste the work that have been done by prior contributors to gather them, or having to reinvent it later (possibly in a breaking manner) to satisfy these requirements from real world use cases. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #61478 +/- ##
==========================================
- Coverage 89.73% 89.67% -0.07%
==========================================
Files 674 689 +15
Lines 204360 210370 +6010
Branches 39274 40092 +818
==========================================
+ Hits 183390 188656 +5266
- Misses 13303 13997 +694
- Partials 7667 7717 +50
🚀 New features to boost your workflow:
|
|
And why not something like OPFS aka whatwg/fs? const rootHandle = await navigator.storage.getDirectory()
await rootHandle.getFileHandle('config.json', { create: true })
fs.mount('/app', rootHandle) // to make it work with fs
fs.readFileSync('/app/config.json')OR const rootHandle = await navigator.storage.getDirectory()
await rootHandle.getFileHandle('config.json', { create: true })
fs.readFileSync('sandbox:/config.json')fs.createVirtual seems like something like a competing specification |
5e317de to
977cc3d
Compare
I generally prefer not to interleave with WHATWG specs as much as possible for core functionality (e.g., SEA). In my experience, they tend to perform poorly on our codebase and remove a few degrees of flexibility. (I also don't find much fun in working on them, and I'm way less interested in contributing to that.) On an implementation side, the core functionality of this feature will be identical (technically, it's missing writes that OPFS supports), as we would need to impact all our internal fs methods anyway. If this lands, we can certainly iterate on a WHATWG-compatible API for this, but I would not add this to this PR. |
|
Small prior art: https://github.com/juliangruber/subfs |
8d711c1 to
73c18cd
Compare
|
I also worked on this a bit on the side recently: Qard@73b8fc6 That is very much in chaotic ideation stage with a bunch of LLM assistance to try some different ideas, but the broader concept I was aiming for was to have a module.exports = new VirtualFileSystem(new LocalProvider())I intended for it to be extensible for a bunch of different interesting scenarios, so there's also an S3 provider and a zip file provider there, mainly just to validate that the model can be applied to other varieties of storage systems effectively. Keep in mind, like I said, the current state is very much just ideation in a branch I pushed up just now to share, but I think there are concepts for extensibility in there that we could consider to enable a whole ecosystem of flexible storage providers. 🙂 Personally, I would hope for something which could provide both read and write access through an abstraction with swappable backends of some variety, this way we could pass around these virtualized file systems like objects and let an ecosystem grow around accepting any generalized virtual file system for its storage backing. I think it'd be very nice for a lot of use cases like file uploads or archive management to be able to just treat them like any other readable and writable file system. |
just a bit off topic... but this reminds me of why i created this feature request: Would not lie, it would be cool if NodeJS also provided some type of static example that would only work in NodeJS (based on how it works internally) const size = 26
const blobPart = BlobFrom({
size,
stream (start, end) {
// can either be sync or async (that resolves to a ReadableStream)
// return new Response('abcdefghijklmnopqrstuvwxyz'.slice(start, end)).body
// return new Blob(['abcdefghijklmnopqrstuvwxyz'.slice(start, end)]).stream()
return fetch('https://httpbin.dev/range/' + size, {
headers: {
range: `bytes=${start}-${end - 1}`
}
}).then(r => r.body)
}
})
blobPart.text().then(text => {
console.log('a-z', text)
})
blobPart.slice(-3).text().then(text => {
console.log('x-z', text)
})
const a = blobPart.slice(0, 6)
a.text().then(text => {
console.log('a-f', text)
})
const b = a.slice(2, 4)
b.text().then(text => {
console.log('c-d', text)
})An actual working PoC (I would not rely on this unless it became officially supported by nodejs core - this is a hack) const blob = new Blob()
const symbols = Object.getOwnPropertySymbols(blob)
const blobSymbol = symbols.map(s => [s.description, s])
const symbolMap = Object.fromEntries(blobSymbol)
const {
kHandle,
kLength,
} = symbolMap
function BlobFrom ({ size, stream }) {
const blob = new Blob()
if (size === 0) return blob
blob[kLength] = size
blob[kHandle] = {
span: [0, size],
getReader () {
const [start, end] = this.span
if (start === end) {
return { pull: cb => cb(0) }
}
let reader
return {
async pull (cb) {
reader ??= (await stream(start, end)).getReader()
const {done, value} = await reader.read()
cb(done ^ 1, value)
}
}
},
slice (start, end) {
const [baseStart] = this.span
return {
span: [baseStart + start, baseStart + end],
getReader: this.getReader,
slice: this.slice,
}
}
}
return blob
}currently problematic to do: also need to handle properly clone, serialize & deserialize, if this where to be sent of to another worker - then i would transfer a MessageChannel where the worker thread asks main frame to hand back a transferable ReadableStream when it needs to read something. but there are probably better ways to handle this internally in core with piping data directly to and from different destinations without having to touch the js runtime? - if only getReader could return the reader directly instead of needing to read from the ReadableStream using js? |
Add note that VFS methods accept the same argument types as their fs counterparts, including string, Buffer, TypedArray, and DataView.
- Remove lib/internal/vfs/entries.js which was dead code (not imported anywhere in the codebase) - Add tests for ENOTEMPTY error (rmdir on non-empty directory) - Add tests for EEXIST error (mkdir on existing directory) PR-URL: nodejs#61478
- Remove unused isVirtualFd() function from fd.js - Remove unused getOpenFdCount() function from fd.js - Remove unused kDefaultBlockSize export from stats.js (still used internally) These functions were exported but never used anywhere in the codebase. PR-URL: nodejs#61478
Remove unused methods from VirtualFD class: - get path() - never accessed - getContentSync() - never called - getContent() - never called Also removes the unused path parameter from VirtualFD constructor and openVirtualFd function. PR-URL: nodejs#61478
Remove additional unused code from VirtualFD class: - get position() / set position() - never accessed - get flags() - never accessed - kFlags symbol - no longer needed - flags parameter from constructor and openVirtualFd() fd.js reduced from 162 to 87 lines. PR-URL: nodejs#61478
- MockFSContext: use public properties instead of private + getters - MockFSContext: use ctx.restore instead of restoreFS - streams.js: use kEmptyObject for default options parameter - streams.js: use getLazy for fd module import - streams.js: remove unnecessary createVirtualReadStream wrapper - file_system.js: use VirtualReadStream class directly PR-URL: nodejs#61478
Add tests to improve code coverage for VFS implementation: - test-vfs-basic.js: - Add tests for mounting at root '/' - Add tests for deeply nested paths - Add tests for truncateSync (shrink and extend) - Add tests for async truncate - Add tests for rename and copyFile operations - test-vfs-real-provider.js: - Add tests for RealFileHandle read/write operations - Add tests for async file handle operations (read, write, stat, truncate, close) - Add tests for recursive mkdir, lstatSync - Add tests for async operations (lstat, copyFile, mkdir, rmdir, rename, readdir, unlink) Both test files now use --expose-internals flag to access internal modules. PR-URL: nodejs#61478
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
- Use kEmptyObject instead of {} for default options
- Add validateObject for options parameter validation
- Use isURL() instead of instanceof URL
- Use toPathIfFileURL() instead of manual URL handling
- Use FunctionPrototypeCall instead of .call()
Users should use sea.getVfs() from node:sea instead of creating SEAProvider directly. getVfs() returns null when not running as SEA, making hasSeaAssets() redundant. - Remove SEAProvider from node:vfs exports - Remove hasSeaAssets() from node:sea - Simplify embedding.js to just check getVfs() result - Update documentation and tests
Users should use the standard writeFileSync/mkdirSync methods instead. MockFSContext retains addFile/addDirectory as convenience methods that prepend the prefix automatically.
Co-authored-by: James M Snell <jasnell@gmail.com>
The SEA VFS is automatically initialized and mounted at /sea. Users can access bundled assets directly via standard fs operations without needing to call any initialization function.
VirtualFileSystem now supports the Explicit Resource Management proposal. The mount() method returns the VFS instance, allowing use with the 'using' declaration for automatic cleanup when leaving scope.
|
@mcollina I'm seeing a lot of functions that can be replaced with |
|
What is the status/progress on this PR right now? I'm going to do another review soon. Likely focussed on docs and tests to start with, before moving on to actual implementation. |
|
I've addressed all feedbacks. |
Ethan-Arrowood
left a comment
There was a problem hiding this comment.
This is looking great. I reviewed docs files, test files, and some miscellaneous bits this round. Leaving pretty much just lib/ as the only directory I haven't reviewed. I will get to that next.
Overall it's looking really good. I'm noticing what is likely some AI leftovers as well as maybe some misorganization of tests. Otherwise, tests are generally very solid. Excited to see it coming together.
doc/api/test.md
Outdated
| * Returns: {MockFSContext} An object that can be used to manage the mock file | ||
| system. | ||
|
|
||
| This function creates a mock file system using the Virtual File System (VFS). |
There was a problem hiding this comment.
Lets link to the vfs docs from here.
test/parallel/test-runner-mock-fs.js
Outdated
| }); | ||
|
|
||
| // Test mock.fs() with dynamic file content | ||
| // TODO(vfs): Dynamic file content (functions) not yet supported in provider-based VFS |
There was a problem hiding this comment.
Is this TODO still true? I believe this is at least documented as supported. I haven't reviewed source yet only docs and now tests.
test/parallel/test-vfs-basic.js
Outdated
| assert.strictEqual(content.slice(0, 2).toString(), 'hi'); | ||
| } | ||
|
|
||
| // Test async truncate |
There was a problem hiding this comment.
This should be in the promises test file, right?
test/parallel/test-vfs-chdir.js
Outdated
| const originalCwd = process.cwd(); | ||
|
|
||
| // Before chdir, process.cwd returns real cwd | ||
| assert.strictEqual(process.cwd(), originalCwd); |
There was a problem hiding this comment.
I don't see much purpose in this assertion.
test/parallel/test-vfs-chdir.js
Outdated
| // After unmount, cwd should throw (not enabled) | ||
| // Actually, virtualCwdEnabled is still true, just unmounted | ||
| // Let's remount and check cwd is reset |
There was a problem hiding this comment.
What are these comments about?
test/parallel/test-vfs-overlay.js
Outdated
|
|
||
| if (isMainThread) { | ||
| // Main thread: create worker with overlay VFS test | ||
| const workerCode = ` |
There was a problem hiding this comment.
I don't love storing code in a string because we can't exactly format or lint it. I'd rather see this be split out into its own test file.
There was a problem hiding this comment.
This file doesn't cover everything the test-vfs-basic.js does. We should have at least one test for every callback and promise form of these methods.
There was a problem hiding this comment.
Something has happened here. Looks like there are tests about just regular methods copyFileSync and what not and this isn't all about provider-memory
There was a problem hiding this comment.
this is what I expected the test-vfs-provider-memory.js file to be more like
There was a problem hiding this comment.
Can you add a test for what happens when you require ESM?
- Separate readFileSync calls in SEA VFS example for clarity - Make getSeaVfs() throw ERR_INVALID_ARG_VALUE if called with a different prefix than the first call, preventing subtle bugs PR-URL: nodejs#61478
Separate initialization from retrieval for cleaner API: - initSeaVfs(options): Initialize with custom options, throws if called twice (ERR_INVALID_STATE) - getSeaVfs(): Get VFS instance, auto-initializes with defaults if not yet initialized This is an internal-only API change; the public behavior remains the same (SEA VFS auto-initializes at startup). PR-URL: nodejs#61478
- Add VFS docs link in test.md mock-fs section - Update TODO precision in test-runner-mock-fs.js - Move async truncate test to test-vfs-promises.js - Expand promise tests with full method coverage - Remove trivial assertion and clean up comments in test-vfs-chdir.js - Add ESM cache explanation in test-vfs-import.mjs - Split overlay worker code into test/fixtures/vfs-overlay-worker.js - Reorganize test-vfs-provider-memory.js to provider-specific tests - Add ESM require tests to test-vfs-require.js
Remove normalizePath and joinMountPath from router.js in favor of path.normalize, path.resolve, and path.join. Remove unused VFS_FD_BASE export from fd.js.
A first-class virtual file system module (
node:vfs) with a provider-based architecture that integrates with Node.js's fs module and module loader.Key Features
Provider Architecture - Extensible design with pluggable providers:
MemoryProvider- In-memory file system with full read/write supportSEAProvider- Read-only access to Single Executable Application assetsVirtualProvider- Base class for creating custom providersStandard fs API - Uses familiar
writeFileSync,readFileSync,mkdirSyncinstead of custom methodsMount Mode - VFS mounts at a specific path prefix (e.g.,
/virtual), clear separation from real filesystemModule Loading -
require()andimportwork seamlessly from virtual filesSEA Integration - Assets automatically mounted at
/seawhen running as a Single Executable ApplicationFull fs Support - readFile, stat, readdir, exists, streams, promises, glob, symlinks
Example
SEA Usage
When running as a Single Executable Application, bundled assets are automatically available:
Public API
Disclaimer: I've used a significant amount of Claude Code tokens to create this PR. I've reviewed all changes myself.