module: use monkey-patchable readFileSync

[merceyz]

Through monkey-patching fs and module it's possible to make Node
support files in zip archives for commonjs, combined with the
experimental loaders the same is possible for ESM. However one line
prevents this from working when importing a commonjs file from an ESM
file. To fix that this PR updates the commonjs translator to use the
monkey-patchable fs.readFileSync instead of destructuring
readFileSync from fs.

Refs: yarnpkg/berry#2161

[aduh95]

The current trend on Node.js codebase has been to make Node.js internals temper-proof, so users can monkey-patch those modules without affecting core built-ins. This PR goes the opposite way, so I'm not sure it's the way we should handle that, or if there could be a different solution to allow to tweak the translator without making it rely on user-mutable methods.
/cc @nodejs/loaders

[bmeck]

This lack of monkey patching is intentional; monkey patching internals has never been directly encouraged or supported. I'm not going to support this PR as it locks internal implementation details into a state that cannot be altered. ESM in particular changes more often than most parts of Node core including but not limited to a desire for some people to refactor from a synchronous loading API to an asynchronous one and moving the loading API off the main thread. Both of those would be problematic for the intent of this PR, which is to allow altering loading behavior. Ongoing efforts to support an official API are being done in the Loaders WG. Due to the nature of this conflicting with Loaders I would suggest attending Loaders calls around it and exposing something that suites your needs. Agreeing upon a feature and documenting it so that support is continued should be a necessary change to this PR.

[GeoffreyBooth]

One of our goals is to allow mocking of modules, including of builtins; but only via loaders, which are privileged in that users need to explicitly enable them. Please take a look at https://github.com/nodejs/loaders/ and the various docs there, especially https://github.com/nodejs/loaders/blob/main/doc/use-cases.md. If you’d like to add your use case to that PR, it would be welcome. And to echo @bmeck, please join a loaders working group meeting if you can.

[merceyz]

In that case shouldn't the translator ask the loader for the source of the resource since the loader was the one to provide it after all?

[jasnell]

Very much -1 on this approach for the same reasons already stated by others.

[GeoffreyBooth]

In that case shouldn’t the translator ask the loader for the source of the resource since the loader was the one to provide it after all?

I’m not sure what this is getting at. If you have a use case you’d like to support, please open an issue in https://github.com/nodejs/loaders.

[merceyz]

I'll close this as I was been able to work around it.

It's been a while since I opened this but @arcanis has been in the loader meetings on behalf of Yarn advocating for our needs.

I’m not sure what this is getting at.

@GeoffreyBooth I meant that that this line should use the source from the load hook instead of using readFileSync.