module: open stat/readPackage to mutations

[arcanis]

While the CJS loader uses the fs module to perform most filesystem I/O, it uses two hidden internal primitives for speed: internalModuleStat, and packageJsonReader. Because internalModuleStat is destructured, it cannot be replaced. As a result, tools that experiment with virtual filesystem layers have to reimplement the whole CJS resolution stack. This is problematic because we want to experiment writing VFS with https://github.com/nodejs/single-executable.

This diff aims to provide a way for advanced tooling authors to avoid having to hijack the full resolution (which is much more invasive and dangerous than what this diff allows), while still respecting the wish for speed from Node (ie we're not suggesting to stop using the special internal primitives, but just to expose them the same way we already expose Module._findPaths or Module._resolveFilename).

[nodejs-github-bot]

Review requested:

• @nodejs/modules

[ljharb]

oof, rather than exposing even more underscored properties, could we just not destructure them and do a live lookup from fs?

[merceyz]

could we just not destructure them and do a live lookup from fs?

Ref #39513 and #39711 which attempted that.

[arcanis]

To answer the concern @addaleax had in #39711:

This is not a great long-term solution, since it still means that embedders rely on internals that can break at any second. It’s probably worth spending some time on thinking about what a proper API for this would look like.

This doesn't have vocation to be a "proper API". There are works in progress that intend to eventually make this possible (for example by making the ESM loaders affect the CJS resolution pipeline). These improvements will however take a long time, blocking valuable experimentations until they land (if ever).

My intent with this diff is to unlock the path for such experiments, without necessarily introducing a first-class API - the assumption being that the limited set of people who will leverage those new bindings are also likely advanced enough to recognize that relying on underscored functions is dangerous, and deal with the fallout.

rather than exposing even more underscored properties, could we just not destructure them and do a live lookup from fs?

I'm not attached to this specific implementation; however, internalModuleStat is part of internalBindings('fs'), not require('fs'), so while userland could still monkey-patch it via process.bindings('fs'), it would encourage more dangerous behaviors than what using an underscored property would (imo).

[GeoffreyBooth]

I’m okay with this in theory, but before we land this I’d like to get more than the usual approvals; at least @guybedford and @aduh95 and anyone else familiar with the CommonJS loader. This seems fine as a short-term solution, but I worry about unintended consequences.

We should have a long-term solution sketched out somewhere; and maybe it already is? I know there’s nodejs/loaders#44 that needs more attention, and I started to work in this direction with nodejs/loaders#94. The latter took the inverse approach from this PR: rather than opening various steps of the resolution algorithm for overriding, provide each step as an isolated function so that you could compose a new resolution function yourself that picks and chooses from the steps that make up the internal algorithm.

[mscdex]

the limited set of people who will leverage those new bindings are also likely advanced enough to recognize that relying on underscored functions is dangerous, and deal with the fallout.

I don't agree with this. In a perfect world, yes, people would take on that responsibility but in reality it has historically always been a problem, even for things that were never publicly documented. A good example of this is EventEmitter's _events property or streams' underscore-prefixed internal state properties.

It's not just the author of a module that uses this "hidden" functionality that is usually the problem, it's that once you have public modules that start depending on these underscore-prefixed "features" you can have a long and/or wide dependency chain that ends up with a lot of downstream users complaining when these underscore-prefixed things change in node core, often requiring dependencies throughout the chain to update versions, bundled dependencies, etc. and that is not always an easy or fast process.

[arcanis]

I understand, but that's just more reasons to have those bindings. Use cases don't disappear without them - it's "just" that tooling authors will instead have to reimplement Module._findPath - a function that not only abstracts a lot of behaviors that must all be replicated, but also uses non-trivial internal algorithms that aren't exposed either (like the exports field resolution), making it very likely a mistake will creep in.

By contrast, _stat and _readPackage are very small pieces of code (that one could argue should have been part of the fs API in the first place, rather than being kept in internals) with very limited concerns, unlikely to change (and I say that as someone who has been watching the CJS loader implementation for a long time now).

[mscdex]

I'm not saying that we don't need to find a solution to this particular problem. I'm saying that simply exposing the existing functions (as underscore-prefixed properties) is not how we should be solving the issue (even as an "experiment"). We should instead seek out a well thought out API that is publicly documented and supported.

[arcanis]

What API do you have in mind? I'm fine with moving internalModuleStat into a fs.statTypeSync function with the same behavior, it's generic enough for that, but packageJsonReader is literally "return the file content if it contains the "main" or "type" or "module" or "imports" or "exports" substring anywhere, otherwise return an empty string". It's a very specialized function.

[aduh95]

the limited set of people who will leverage those new bindings are also likely advanced enough to recognize that relying on underscored functions is dangerous, and deal with the fallout.

I don't agree with this. In a perfect world, yes, people would take on that responsibility but in reality it has historically always been a problem, even for things that were never publicly documented. A good example of this is EventEmitter's _events property or streams' underscore-prefixed internal state properties.

It's not just the author of a module that uses this "hidden" functionality that is usually the problem, it's that once you have public modules that start depending on these underscore-prefixed "features" you can have a long and/or wide dependency chain that ends up with a lot of downstream users complaining when these underscore-prefixed things change in node core, often requiring dependencies throughout the chain to update versions, bundled dependencies, etc. and that is not always an easy or fast process.

FWIW this has been recently illustrated by #42623 (comment), a change made in an experimental API (loaders) broke a third party package (Yarn Berry) even though the Yarn team did their best to prevent their users to be affected by releasing a patch version long in advance. Of course I'm grateful that those APIs are being used and tested in the while so we can improve them, but yes I agree with Brian here, we don't live in a perfect world unfortunately.

That being said, CJS loader can already be broken by user-code very easily, I'm don't think this particular change would worsen the situation a lot, but I can be wrong.

[aduh95]

There's a linter failure that would need to be addressed, other than that, LGTM

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/46587/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/46598/

[arcanis]

The CI failure seems unrelated to my diff

[RaisinTen]

@arcanis I was actually working on a test right now, so I sent #44713. Maybe you can take over the Module._readPackage test or add more Module._stat tests? :)