internal: lazy load fs due to circular dependency #11260
Conversation
nodejs-github-bot
added
the
fs
test/parallel/test-stdin-require.js
Outdated
| const path = require('path'); | ||
|
|
||
| if (common.isWindows) { | ||
| common.skip('Skipping test on windows'); |
There was a problem hiding this comment.
Maybe include why it's being skipped? I don't know if we usually do that or not...
There was a problem hiding this comment.
Yea, as of right now, I just don't know how to reproduce on windows and I don't have access to a windows box.
There was a problem hiding this comment.
I'll spend a bit trying to reproduce on windows and if I can't, I'll update the message
test/parallel/test-stdin-require.js
Outdated
| `echo "require('${filename}')" | ${bin} &> ${out} || cat ${out}`; | ||
|
|
||
| const result = execSync(cmd).toString(); | ||
| assert.strictEqual(result.includes('assertEncoding is not a function'), |
There was a problem hiding this comment.
Is there anything we can explicitly detect? I'm just worried that if assertEncoding were to change names and a regression occurred, this wouldn't detect it.
There was a problem hiding this comment.
hm, maybe we can make the require console.log something instead? I'll see if I can come up with a better way of verifying the behavior. Good call :]
|
Another way of fixing this would be to move the implementation of |
The fs module requires internal/fs and internal/fs requires fs which causes a circular dependency. This change lazy loads fs inside of internal/fs to prevent assertEncoding from breaking when requiring via stdin. Fixes: nodejs#11257
|
@jasnell true, but at the same time, aren't we deprecating that? I'm also not sure how much sense it makes to move the implementation of |
|
deprecating The justification for moving them to internal would be precisely to avoid this kind of circular dependency and the need to lazy load at all. Just my opinion tho :-) |
|
@jasnell sorry for not being clear. I meant aren't we deprecating SyncWriteStream? |
|
That's different. |
|
@evanlucas, any updates on this? |
|
I think this could be closed in favor of the new PR? |
|
I believe it can be. |
The fs module requires internal/fs and internal/fs requires fs which
causes a circular dependency. This change lazy loads fs inside of
internal/fs to prevent assertEncoding from breaking when requiring via
stdin.
Fixes: #11257
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
internal/fs