fs: restore fs patchability in ESM loader

[joyeecheung]

Temporarily restore fs patchability in ESM loader as a workaround for helping downstream projects that depend on this undocumented hidden contract transition into using hook proper APIs. This patch intentionally avoids adding a test and instead adds warning comments to hopefully steer new code away from depending on it.

Refs: #62012

[nodejs-github-bot]

Review requested:

• @nodejs/loaders

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.63%. Comparing base (dfe438d) to head (79a7b4c).
⚠️ Report is 60 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #62835      +/-   ##
==========================================
- Coverage   89.81%   89.63%   -0.19%     
==========================================
  Files         699      706       +7     
  Lines      216379   219148    +2769     
  Branches    41366    41981     +615     
==========================================
+ Hits       194340   196424    +2084     
- Misses      14139    14617     +478     
- Partials     7900     8107     +207     

Files with missing lines	Coverage Δ
lib/internal/modules/esm/load.js	91.62% <100.00%> (+0.15%)	⬆️
lib/internal/modules/esm/resolve.js	99.04% <100.00%> (+<0.01%)	⬆️
lib/internal/modules/esm/translators.js	97.68% <100.00%> (+0.01%)	⬆️

... and 77 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[mcollina]

lgtm

[nodejs-github-bot]

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

[jsumners-nr]

Is this saying that any developer that needs to verify code flowing through fs will need to devise a hook?

Consider https://github.com/pinojs/pino-pretty/blob/d7138e1e198ab2227a58be6b41ff248f2ad39765/test/basic.test.js#L1088-L1112

test('does not call fs.close on stdout stream', (t) => {
    t.plan(2)
    const destination = pino.destination({ minLength: 4096, sync: true })
    const prettyDestination = pinoPretty({ destination, colorize: false })
    const log = pino(prettyDestination)
    log.info('this message has been buffered')
    const chunks = []
    const { close, writeSync } = fs
    let closeCalled = false
    fs.close = new Proxy(close, {
      apply: (target, self, args) => {
        closeCalled = true
      }
    })
    fs.writeSync = new Proxy(writeSync, {
      apply: (target, self, args) => {
        chunks.push(args[1])
        return args[1].length
      }
    })
    destination.end()
    Object.assign(fs, { close, writeSync })
    t.assert.match(chunks.join(''), /INFO .+: this message has been buffered/)
    t.assert.strictEqual(closeCalled, false)
  })

This is inlined and easy to read. I imagine that requiring the same test to go through a hook will be much more difficult to follow and implement.

[joyeecheung]

This is about module loaders picking up fs or not. In the example you raise the question is whether pino picks up fs or not and that would be orthogonal to our comments here. But yes in general you can't count on random external code to always use certain fs methods for fs operations. Just like Node.js module loaders could be re-written in C++ and there won't be any guarantee that it must reuse any user-patchable public JS APIs in the implementation (for example it already doesn't use any public APIs to read package.json). Only the module hooks are guaranteed to be invoked no matter how the underlying implementation changes.

[jsumners-nr]

🤔