[v10.x] fs: stop lazy loading stream constructors #21776
Conversation
nodejs-github-bot
added
the
fs
There was a problem hiding this comment.
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.
| @@ -1793,43 +1778,12 @@ module.exports = fs = { | |||
| writeSync, | |||
| Stats, | |||
|
|
|||
| get ReadStream() { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That's how it's implemented now. It doesn't work with jest-mock because accessors are ignored.
There was a problem hiding this comment.
Ah right, I see that there are setters...so this looks more like a bug of Jest?
There was a problem hiding this comment.
Yes, maybe. But this was still kind of a sever major change and I'm not sure it's worth to do it.
|
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? |
| function lazyFs() { | ||
| if (fs === undefined) | ||
| fs = require('fs'); | ||
| return fs; |
There was a problem hiding this comment.
Why not to just return require('fs') since require already has a cache?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@jasnell we might want to benchmark the influence of that again. It would actually be so clean if we'd just use require directly.
|
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 🙂 |
|
@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. |
|
I'm good with this change in v10 but keeping the current behavior in master and 11. |
|
Specifically, what I'd like to see is this PR rebased against |
targos
changed the title
|
Updated to target |
|
@joyeecheung are you fine with this landing on v10.x only? |
|
Landed in 484140e |
Fixes: #21489
/cc @nodejs/fs @SimenB
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes