Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug?]: fs.rm is broken with recursive option in yarn 3 pnp, node >= 20.12.0 #6247

Open
1 task
jwoo0122 opened this issue Apr 24, 2024 · 4 comments
Open
1 task
Labels
bug Something isn't working

Comments

@jwoo0122
Copy link

jwoo0122 commented Apr 24, 2024

Self-service

  • I'd be willing to implement a fix

Describe the bug

When using fs.rm with recursive: true (or its sync, async version rmSync and fs/promises#rm), there's an error of TypeError [ERR_INVALID_ARG_TYPE]: The "list[2]" argument must be an instance of Buffer or Uint8Array. Received type string.

To reproduce

  1. Clone reproduction repository https://github.com/jwoo0122/yarn-3-node-20-test
  2. Run yarn node test.js

Environment

System:
    OS: macOS 14.2.1
    CPU: (10) arm64 Apple M1 Max
  Binaries:
    Node: 20.12.2 - /private/var/folders/l4/c6zfhs7d0zq4lm_fgkrlml1c0000gq/T/xfs-d0b6f4b6/node
    Yarn: 3.8.1 - /private/var/folders/l4/c6zfhs7d0zq4lm_fgkrlml1c0000gq/T/xfs-d0b6f4b6/yarn
    npm: 10.5.0 - ~/.nvm/versions/node/v20.12.2/bin/npm
    bun: 1.0.27 - ~/.bun/bin/bun

Additional context

I found this doesn't happen on node 18. recursive option of fs.rm uses readdirSync, and on Node 18 we use pure node fs module but on Node 20 we use pnp patched version.

This error happens when the patched readdirSync is used internally, by converting the result from Buffer to string with NodePathFS.mapToBase. It also means that pnp patched readdirSync does not respect the encoding option, which is buffer in internal/fs/rimraf. According to Node.js Docs if the encoding option set to buffer, the result should be an array of buffer.

@jwoo0122 jwoo0122 added the bug Something isn't working label Apr 24, 2024
@jwoo0122 jwoo0122 changed the title [Bug?]: fs.rm is broken in yarn 3.8.1, node 20.12.1 [Bug?]: fs.rm is broken with recursive option in yarn 3.8.1, node 20.12.1 Apr 24, 2024
@jwoo0122
Copy link
Author

From Node 20.12.0, ESM facade is build when it's necessary. Before this change (like, <=Node 20.11.1) when the fs module is imported, internal/fs/rimraf was also resolved.

In .pnp.cjs of yarn 3.8.1, we do require('fs'). At this point fs is not patched at all. So this happens:

  • If you're using Node.js <20.12.0, internal/fs/rimraf is resolved by getESMFacade. It uses non-pnp version of fs.readdirSync (original version), which can handle encoding option. No Error.
  • If you're using Node.js >=20.12.0, internal/fs/rimraf is not resolved. After we meet the code in user script which actually use internal/fs/rimraf (e.g fs.rmSync), node resolves the module. At this point we already done the preloading of pnp patching, so internal/fs/rimraf uses pnp-patched version of fs.readdirSync, which doesn't handle encoding option. But internal/fs/rimraf wants it to return the result as buffer array, so there's an Error.

@jwoo0122
Copy link
Author

jwoo0122 commented Apr 24, 2024

Looks like that yarn 4 NodeFS.readdir(Sync) supports the options correctly, but not for <3. So this error happens when you use Node.js 20.12.0 or later && yarn 3.x or earlier.

@jwoo0122 jwoo0122 changed the title [Bug?]: fs.rm is broken with recursive option in yarn 3.8.1, node 20.12.1 [Bug?]: fs.rm is broken with recursive option in yarn 3, node > 20.12.0 Apr 24, 2024
@jwoo0122 jwoo0122 changed the title [Bug?]: fs.rm is broken with recursive option in yarn 3, node > 20.12.0 [Bug?]: fs.rm is broken with recursive option in yarn 3, node >= 20.12.0 Apr 24, 2024
@jwoo0122 jwoo0122 changed the title [Bug?]: fs.rm is broken with recursive option in yarn 3, node >= 20.12.0 [Bug?]: fs.rm is broken with recursive option in yarn 3 pnp, node >= 20.12.0 Apr 25, 2024
@sotojn
Copy link

sotojn commented Sep 26, 2024

Is there any way to resolve this or is there a later yarn 3 patch that addresses this? I am having the same issue currently after updating node in my project. Am I forced to update to yarn 4? I'd prefer not to but I was curious what my options were.

@jwoo0122
Copy link
Author

Is there any way to resolve this or is there a later yarn 3 patch that addresses this? I am having the same issue currently after updating node in my project. Am I forced to update to yarn 4? I'd prefer not to but I was curious what my options were.

Not very helpful, but I just downgraded my node version to 20.11.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants