GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Virtual File System for Node.js by mcollina · Pull Request #61478 · nodejs/node Conversation
|
|
Review requested:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| */ | ||
| existsSync(path) { | ||
| // Prepend prefix to path for VFS lookup | ||
| const fullPath = this.#prefix + (StringPrototypeStartsWith(path, '/') ? path : '/' + path); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Can we use path.join?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| validateObject(files, 'options.files'); | ||
| } | ||
|
|
||
| const { VirtualFileSystem } = require('internal/vfs/virtual_fs'); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we import this at the top level / lazy load it at the top level?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| ArrayPrototypePush(this.#mocks, { | ||
| __proto__: null, | ||
| ctx, | ||
| restore: restoreFS, |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| restore: restoreFS, | |
| restore: ctx.restore, |
nit
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| * @param {object} [options] Optional configuration | ||
| */ | ||
| addFile(name, content, options) { | ||
| const path = this._directory.path + '/' + name; |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Can we use path.join?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| let entry = current.getEntry(segment); | ||
| if (!entry) { | ||
| // Auto-create parent directory | ||
| const dirPath = '/' + segments.slice(0, i + 1).join('/'); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Let's use path.join
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| let entry = current.getEntry(segment); | ||
| if (!entry) { | ||
| // Auto-create parent directory | ||
| const parentPath = '/' + segments.slice(0, i + 1).join('/'); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
path.join?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| } | ||
| } | ||
| callback(null, content); | ||
| }).catch((err) => { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| }).catch((err) => { | |
| }, (err) => { |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| const bytesToRead = Math.min(length, available); | ||
| content.copy(buffer, offset, readPos, readPos + bytesToRead); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Primordials?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| } | ||
|
|
||
| callback(null, bytesToRead, buffer); | ||
| }).catch((err) => { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| }).catch((err) => { | |
| }, (err) => { |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Left an initial review, but like @Ethan-Arrowood said, it'll take time for a more in depth look |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@ ## main #61478 +/- ## ========================================== + Coverage 89.68% 89.80% +0.11% ========================================== Files 676 692 +16 Lines 206555 215773 +9218 Branches 39552 41299 +1747 ========================================== + Hits 185249 193767 +8518 - Misses 13444 14118 +674 - Partials 7862 7888 +26
🚀 New features to boost your workflow:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Small prior art: https://github.com/juliangruber/subfs |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
8d711c1 to
73c18cd
Compare
There was an error while loading. Please reload this page.
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
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? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work, Matteo. I am not sure if I understand the point of per-VFS working directories, especially if this mechanism is also going to hook into process.chdir(). I think it'd be reasonable to only allow path resolution relative to the virtual root within a VFS. The interaction between this feature and child processes, which inherit the Node.js working directory, also does not seem to have a clear solution.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| ### Native addons | ||
|
|
||
| Native addons (`.node` files) cannot be loaded from the VFS. Native addons | ||
| must exist on the real file system because they are loaded by the operating | ||
| system's dynamic linker, which cannot access virtual files. |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This section should probably also mention that native addons, like child processes, also cannot see virtual file systems.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| If case-insensitive matching is required, applications should normalize paths | ||
| before VFS operations. |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Normalizing paths might not resolve this issue if the application is not responsible for creating the file.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
can you make an example?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| * **Read operations** (`readFile`, `readdir`, `stat`, `lstat`, `access`, | ||
| `exists`, `realpath`, `readlink`, `statfs`, `opendir`): Check VFS first. If | ||
| the path doesn't exist in VFS, fall through to the real file system. | ||
| * **Write operations** (`writeFile`, `appendFile`, `mkdir`, `rename`, `unlink`, | ||
| `rmdir`, `symlink`, `copyFile`, `truncate`, `link`, `chmod`, `chown`, | ||
| `utimes`, `lutimes`, `mkdtemp`, `rm`, `cp`): Always operate on VFS. New | ||
| files are created in VFS, and attempting to modify a real file that doesn't | ||
| exist in VFS will create a new VFS file instead. |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that the behavior of open() depends on the flags that are specified? Is the idea that all write operations (even, for example, O_RDWR without O_CREAT) target the VFS only, so the underlying filesystem should not be modifiable through this?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
That was my initial thought as well. But it seems that you could mount that to a real file system. According to: #62328 (comment)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that the behavior of open() depends on the flags that are specified? Is the idea that all write operations (even, for example, O_RDWR without O_CREAT) target the VFS only, so the underlying filesystem should not be modifiable through this?
A VFS provider can implement those flags as they would please, e.g. S3 operations. In the current codebase, those are passed through the real provider.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
I would like to be able to run a Node.js application inside worker threads, and some of them, unfortunately, do chdir() within dependencies. Supporting chdir() will allow us to do it. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Co-authored-by: Tobias Nießen <[email protected]>
Thanks Matteo, I understand the motivation now. Personally, I don't feel great about retrofitting |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
The same can be said of all the content in the VFS and native addons. Native addons won't be able to "see" inside the vfs. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
My concern is not strictly limited to VFS; any |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I've extracted #63115 from this PR. It includes only the addition and no integration points. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanatory comment
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Closing this. Most of it has been implemented and it landed in core. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
Implement VFS (Virtual File System) Hooks for Single Executable Applications
There was an error while loading. Please reload this page.