internalModuleStat & internalModuleReadJSON prevent resolution override

[arcanis]

What steps will reproduce the bug?

Imagine that the fs module is overriden to add support for a virtual filesystem. Make a require call towards a virtual file.

What is the expected behavior?

The resolution should succeed, since the fs methods know how to access the file.

What do you see instead?

Cannot find module 'virtual'.

Additional information

This occurs because Node cheats and doesn't actually use all of the fs methods. In most cases it does (for example fs.realpath, fs.readFile, fs.readdir), but not for all. Two methods in particular goes into unique native bindings that cannot be overriden:

• internalModuleStat is used instead of fs.stat: https://github.com/nodejs/node/blob/master/lib/internal/modules/cjs/loader.js#L151
• internalModuleReadJSON is used instead of fs.readFile:
  https://github.com/nodejs/node/blob/master/lib/internal/modules/cjs/loader.js#L257

Since those functions aren't exposed (not only do they come from internalBinding, they're also destructured so we never have a chance to change their references), we cannot even add the virtual layer to them at all.

Would it be possible to use the fs primitives or, at least, to expose internalModuleStat & friends in a way we can wrap them?

[bnoordhuis]

Would it be possible to use the fs primitives or, at least, to expose internalModuleStat & friends in a way we can wrap them?

No and no. The module loader doesn't use fs for performance reasons and internalModuleStat should not be exposed because that means setting its API in stone. It needs to be able to evolve as the need arises, without having to worry about backwards compatibility.

I suppose Node could detect when fs is monkey-patched and fall back to using the monkey-patched fs.readFileSync or whatever but that feels wrong on multiple levels. Don't bend require() into something it's not intended for.

[arcanis]

To be clear: I don't want (or really need) internalModuleStat exposed directly through fs. I don't mind this API being private. I get that you don't want to expose two functions that basically do the exact same thing.

That being said, there is currently no way to override this part of the filesystem access, whether it's for instrumentation or other purposes. It's quite unique to this part of Node, and in that there's a decent case that it can be constructed as a bug (just imagine that I never mentioned internalModuleStat and this thread is called "require doesn't return the same results as my manual fs-based implementation").

Basically, all I'm asking is to change:

const {
  internalModuleReadJSON,
  internalModuleStat
} = internalBinding('fs');

Into:

const fsBinding = internalBinding('fs');

Nothing would be any more public than it currently is, but it would help people who know what they're doing at very little cost.

I suppose Node could detect when fs is monkey-patched and fall back to using the monkey-patched fs.readFileSync

I don't think this would be a good idea. It shouldn't be Node's responsibility to account for potential external runtimes, it should be external runtimes that perfectly replicate Node's expected behavior. Problem is, for this to happen, it needs to be technically possible 😃

[pauldraper]

FWIW, Java has figured out how to support virtual file systems (as well as custom class loaders).

https://docs.oracle.com/javase/8/docs/technotes/guides/io/fsp/filesystemprovider.html