Review requested:

• @nodejs/single-executable
• @nodejs/test_runner

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @avivkeller.

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.

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 89.32170% with 710 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.65%. Comparing base (48c208f) to head (0abdb2f).
⚠️ Report is 56 commits behind head on main.

@@            Coverage Diff             @@
##             main   #61478      +/-   ##
==========================================
- Coverage   91.64%   89.65%   -2.00%     
==========================================
  Files         337      691     +354     
  Lines      140455   213208   +72753     
  Branches    21779    40576   +18797     
==========================================
+ Hits       128716   191144   +62428     
- Misses      11517    14175    +2658     
- Partials      222     7889    +7667     

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

And why not something like OPFS aka whatwg/fs?

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

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 VirtualFileSystem type which would actually implement the entire API surface of the fs module, accepting a Provider type to delegate the internals of the whole cluster of file system types to a singular class managing the entire cluster of fs-related types such that the fs module could actually just be fully converted to:

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.

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:
Blob.from() for creating virtual Blobs with custom backing storage

Would not lie, it would be cool if NodeJS also provided some type of static Blob.from function to create virtual lazy blobs. could live on fs.blobFrom for now...

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)
})

x-z xyz
a-z abcdefghijklmnopqrstuvwxyz
a-f abcdef
c-d cd

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: new Blob([a, b]), new File([blobPart], 'alphabet.txt', { type: 'text/plain' })

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?

@joyeecheung this should align more of what you are asking for. Thanks for the direct feedback because it seemed an endless chase on the line comments as no direction seemed to go well.

(Sorry for the extra 2000 lines of code in this huge PR).

@joyeecheung:

... no one would be committed enough to fix the quirks ...

I don't think this is a fair statement. @mcollina is obviously invested in working on this, is obviously intending to keep iterating, and is obviously someone we can trust to follow up.

As someone who has had to read through multiple iterations of this massive PR multiple times now, I'd really appreciate if we can get this landed and iterate in smaller steps moving forward.

I don't think this is a fair statement. @mcollina is obviously invested in working on this, is obviously intending to keep iterating, and is obviously someone we can trust to follow up.

I believe so too, but unfortunately module loader has an unusual risk surface in terms of compatibility and maintainability; if the implementation largely reuses existing code (like it currently does), "having someone committed to follow up" may be enough; having a 3rd diverging implementation like this PR once did, however, IMO would pass threshold for human wills to be enough to mitigate the risk and necessitate gates like runtime flags to prevent a 3rd implementation from incurring even more maintenance issues, since 2 diverging duplicate implementations are already difficult to manage and there needs to be a line.

Seems the vfs-mount event was removed after I pointed out it was insufficient as a security gate, but no permission system equivalent was ever added in its place. I'm not sure if we should land this until sufficient permission controls are in-place as it seems like it would be a breaking change to permission-gate that capability later.

Am I the only one here concerned about the security implications of being able to shadow file system access across the entire thread from any loaded JS file? Malicious dependencies could easily hijack file loads in just a few lines. They could technically do that already with a bunch of patching of the file system module, but this API makes it basically trivial. I'll concede if TSC folks think this is outside our threat model, but even so it seems a bit troubling to me. 🤔

Malicious dependencies could easily hijack file loads in just a few lines.

Somewhere in this thread I asked @RafaelGSS to verify and Rafael concludes that as long as it's in-memory it's okay. The VFS stuff only allows user code to change where the sys calls go to at most, but when it gets to actually invoking the sys calls, that will still be gated by permission model e.g. VFS allows redirecting /virtual/path -> /path, but actually making system calls to read from /path can still be still gated.

Even then, note that our threat model states that permission models are...

...designed to reduce the blast radius of mistakes in trusted application code, not to act as a security boundary against intentional misuse or a compromised process.