[v10.x] fs: stop lazy loading stream constructors

[targos]

Fixes: #21489

/cc @nodejs/fs @SimenB

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

[nodejs-github-bot]

@targos build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/255/pipeline

[joyeecheung]

Do we need to add a test to make sure these public members of fs are mockable (i.e. if you assign fs.member to something else then assert.strictEqual(fs.member, mockedMember)? It should be less useful for other modules but members of fs are mocked by users all the time in tests.

[joyeecheung]

An alternative fix is adding setters here to allow users to override the lazy-loaded variables so we can still avoid loading the internal/fs/streams module when requiring fs.

[targos]

That's how it's implemented now. It doesn't work with jest-mock because accessors are ignored.

[joyeecheung]

Ah right, I see that there are setters...so this looks more like a bug of Jest?

[targos]

Yes, maybe. But this was still kind of a sever major change and I'm not sure it's worth to do it.

[joyeecheung]

Should we just wait until Jest fixes the bug instead of working around it ourselves? Looks like Jest just need to consider properties with setters as properties that can be overridden instead of bypassing every property with a getter in the descriptor?

[joyeecheung]

Putting a red cross because it does not look like it has to be us fixing the regression, since we already provide setters (also because GitHub does not allow you to dismiss approvals..). This can be dismissed if it's shown that it has to be us fixing this.

[vitkarpov]

Why not to just return require('fs') since require already has a cache?

[jasnell]

The require cache is slightly more expensive so we generally use this pattern through core to keep it at cheap as possible. It's likely unnecessary in most cases, but it's a consistent pattern.

[vitkarpov]

Okay, cool. Thank you!

[BridgeAR]

@jasnell we might want to benchmark the influence of that again. It would actually be so clean if we'd just use require directly.

[SimenB]

I'm fine with fixing this in Jest (although I won't have time for it anytime soon, leaving for vacation), but it has over 1.3 million every week, and people are generally slow to update. It also broke in a minor, not a major (and the original PR was originally marked as major before passing CITGM).

That said, nobody has complained to us about it not working, only noticed since our own tests broke 🙂

[BridgeAR]

@joyeecheung I personally would say it was a unintentionally breaking change. It was the reason why I did not move that part out when I was working on improving lazy loading in Node core.

So I am +1 on merging this at least on 10.

[jasnell]

I'm good with this change in v10 but keeping the current behavior in master and 11.

[jasnell]

Specifically, what I'd like to see is this PR rebased against v10.x-staging, rather than against master.

[targos]

Updated to target v10.x-staging. PTAL

[targos]

@joyeecheung are you fine with this landing on v10.x only?

[targos]

CI: https://ci.nodejs.org/job/node-test-pull-request/15922/
Edit: resumed build: https://ci.nodejs.org/job/node-test-pull-request/15941/

[targos]

Landed in 484140e