[v18.x backport] fs: fix the file name is not included when the withFileTypes option of readdir

[sonsurim]

This fixes the issue reported in #52441

The issue was caused by a modification in PR #51021 , which was working on including the parentPath in the documentation and modifying the path to display the filepath.

I have retained the content related to the documentation and removed the filepath part. Corresponding test code has also been updated accordingly.

An error occurred in the test/sequential/test-fs-opendir-recursive.js file after changing the code. This was resolved by referring to the code resolved in #49603 .

cc. @H4ad @daeyeon

Screenshots

AS-IS
You can see that even the file name is exposed in the path.

{
  const dir = await readdir(tmpDir, { withFileTypes: true });
  console.log(dir);
}

TO-BE
Now the file name has been removed from the path!

{
  const dir = await readdir(tmpDir, { withFileTypes: true });
  console.log(dir);
}

References

• [v18.x] Wrong path returned by fs.readdir withFileTypes=true #52441
• fs: introduce dirent.parentPath #50976
• [v18.x backport] fs: introduce dirent.parentPath #51021
• fs: fix readdir and opendir recursive with unknown file types #49603

[daeyeon]

Congrats on your first contribution! 🎉
Please update the commit message per the guide to fix the CI issue. Also, remove me as a co-author since I didn't directly contribute and don't qualify. :-)

[sonsurim]

The git config was not set up properly, so I committed it again. 😅

[aduh95]

This one is simply very hard to fix without introducing more breaking changes. It might be more wise to consider dirent.path irremediably broken and switch to using dirent.parentPath which is stable.

[aduh95]

This change is not OK, we cannot make this kind of breaking change, especially on a maintenance LTS line. This needs to remain as it was on v18.19.1:

node/test/sequential/test-fs-opendir-recursive.js

Line 131 in 6610cc4

return pathModule.relative(testDir, dirent.path);

[sonsurim]

I got it! I fixed in b2d61d8.
Many Thanks!

[sonsurim]

@aduh95 Thanks for the review!

It might be more wise to consider dirent.path irremediably broken and switch to using dirent.parentPath which is stable.

Does the above mean modifying the readSyncRecursive function to use parentPath instead of path?

// dir.js
readSyncRecursive(dirent) {
    const ctx = { path: dirent.parentPath };
    const handle = dirBinding.opendir(
      pathModule.toNamespacedPath(dirent.parentPath),
      this[kDirOptions].encoding,
      undefined,
      ctx,
    );
    handleErrorFromBinding(ctx);
    const result = handle.read(
      this[kDirOptions].encoding,
      this[kDirOptions].bufferSize,
      undefined,
      ctx,
    );

    if (result) {
      this.processReadResult(dirent.parentPath, result);
    }

    handle.close(undefined, ctx);
    handleErrorFromBinding(ctx);
  }

Could you please help me a little more?

[aduh95]

Does the above mean modifying the readSyncRecursive function to use parentPath instead of path?

No I meant for user land, Im unsure fixing it in Node.js is worth it

[H4ad]

This one is simply very hard to fix without introducing more breaking changes. It might be more wise to consider dirent.path irremediably broken and switch to using dirent.parentPath which is stable.

Doesn't make sense this statement since on the sync version we have the correct behavior, unless you want to change the sync version.

[aduh95]

unless you want to change the sync version.

No that would be equally unwise IMO. What I'm saying is that dirent.path is inconsistent, and I don't see a way to fix the inconsistency without risking breaking more stuff. dirent.parentPath was introduced because dirent.path is inconsistent and unfortunately the ecosystem was/is relying on some of these inconsistencies.
If there's a way to bring back the behavior of v18.19.1 without breaking dirent.parentPath, I think we should consider doing so; I couldn't figure a way for doing so, I'm not saying it's impossible, but given the fact that v18.19.1 behavior is also inconsistent, I'm not sure it's worth doing.

[sonsurim]

I will wait for the direction of the work to be decided. If you decide, please let me know!