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

fs.readdir's new recursive option is not fully documented #48640

Closed
That-Guy977 opened this issue Jul 3, 2023 · 14 comments · Fixed by #48902
Closed

fs.readdir's new recursive option is not fully documented #48640

That-Guy977 opened this issue Jul 3, 2023 · 14 comments · Fixed by #48902
Labels
confirmed-bug Issues with confirmed bugs. doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system.

Comments

@That-Guy977
Copy link

That-Guy977 commented Jul 3, 2023

Affected URL(s)

https://nodejs.org/dist/latest-v20.x/docs/api/fs.html#fspromisesreaddirpath-options and callback/synchronous versions

Description of the problem

From testing, the recursive option does not work with withFileTypes: true.
The behavior of the recursive option is not documented, and only recursive's type and default value are present in the documentation. The callback-async fs.readdir and the synchronous fs.readdirSync yield erraneous results (entries missing or appear with missing fields) when both recursive and withFileTypes are supplied.

This issue tracks missing documentation. Incorrect behavior is tracked at #48858.

@That-Guy977 That-Guy977 added the doc Issues and PRs related to the documentations. label Jul 3, 2023
@anonrig
Copy link
Member

anonrig commented Jul 4, 2023

cc @Ethan-Arrowood

@MoLow MoLow added the good first issue Issues that are suitable for first-time contributors. label Jul 4, 2023
@pernelkanic
Copy link

can i work on this?

@Ethan-Arrowood
Copy link
Contributor

Can you share the case where it doesn't work with withFileTypes? I added a multitude of tests when I landed the option so I wanna know what I missed

@targos
Copy link
Member

targos commented Jul 4, 2023

Btw it looks more like a bug than a documentation issue.

@targos targos added the fs Issues and PRs related to the fs subsystem / file system. label Jul 4, 2023
@MoLow MoLow removed the good first issue Issues that are suitable for first-time contributors. label Jul 4, 2023
@That-Guy977
Copy link
Author

Btw it looks more like a bug than a documentation issue.

No documentation is provided for recursive regardless, so I think it's still an issue. Maybe I should have filed the lack of documentation and the bug separately.

@ghost
Copy link

ghost commented Jul 5, 2023

Can I work on this, it will be my first issue.

@That-Guy977
Copy link
Author

That-Guy977 commented Jul 5, 2023

@Ethan-Arrowood From further testing, I've found some behavior that varies between the promise, callback, and synchronous versions.

  • The promise version appears to not have any issues.

  • The callback and synchronous versions can generate a entry Dirent { name: undefined, path: '...', [Symbol(type)]: undefined }, when withFileTypes is supplied, replacing a valid entry.

  • The callback and synchronous versions fail to list certain files and directories when both withFileTypes and recursive are provided. I noticed this as several entries under .git missing, I'm unsure what the cause is.

    Note: Since nested directories themselves are not listed, these issues are likely not related to the recursion step as I had previously assumed, however this behavior is only present when recursive is also supplied and is not present in previous versions (Tested on Node v18.16.1.). I will update the original message to reflect this.

A minor inconsistency I found is that when providing the results the promise version appears to act as depth-first, while the callback and synchronous versions act as breath-first in their resulting arrays.

These tests have been on a limited filetree which I've uploaded to fs-readdir-recursive.

@Ethan-Arrowood
Copy link
Contributor

Okay I can confirm there are two issues here.

First, missing documentation for the recursive option for the callback and sync versions.

Second, I am seeing the undefined behavior in the callback and sync versions; I think your latest message @That-Guy977 explains it well. What I'm most intrigued by is how the tests aren't catching this. We definitely need to fix this. I'm going to start investigating this problem now.

Finally, I do not believe the difference in search behavior matters. We make no guarantees in how Node recursively searches a directory. In a long historical thread I commented the possibility of specifying which algorithm could be used, but that is a feature not a bug in this case.

Some users here expressed interest in contributing, @pernelkanic you commented first. Would you like to work on the documentation fix?

@That-Guy977
Copy link
Author

That-Guy977 commented Jul 7, 2023

First, missing documentation for the recursive option for the callback and sync versions.

I believe the promise version has the same lack of documentation, as it and the other versions are lacking a similar description as opendir or rm, or even an explanation in the method description as is present in mkdir.

@Ethan-Arrowood
Copy link
Contributor

Oh good point 👍 that should be fixed too then

@Ethan-Arrowood
Copy link
Contributor

I'm happy to leave the documentation changes for a new contributor, but can also add those in if we get no response in a couple days.

@anonrig anonrig added the confirmed-bug Issues with confirmed bugs. label Jul 7, 2023
@Ethan-Arrowood
Copy link
Contributor

@RamdohokarAngha would you be interested in contributing docs for this?

Ethan-Arrowood pushed a commit to Ethan-Arrowood/node that referenced this issue Jul 19, 2023
@ghost
Copy link

ghost commented Jul 20, 2023

@Ethan-Arrowood - Yes, I'm interested in contributing docs for this issue.

@Ethan-Arrowood
Copy link
Contributor

Ethan-Arrowood commented Jul 20, 2023

Thank you! Go for it!

nodejs-github-bot pushed a commit that referenced this issue Aug 12, 2023
Refs: #48640
PR-URL: #48698
Fixes: #48858
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Ceres6 pushed a commit to Ceres6/node that referenced this issue Aug 14, 2023
Refs: nodejs#48640
PR-URL: nodejs#48698
Fixes: nodejs#48858
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
RafaelGSS pushed a commit that referenced this issue Aug 15, 2023
Refs: #48640
PR-URL: #48698
Fixes: #48858
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
RafaelGSS pushed a commit to RafaelGSS/node that referenced this issue Aug 15, 2023
Refs: nodejs#48640
PR-URL: nodejs#48698
Fixes: nodejs#48858
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
rluvaton pushed a commit to rluvaton/node that referenced this issue Aug 15, 2023
Refs: nodejs#48640
PR-URL: nodejs#48698
Fixes: nodejs#48858
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
RafaelGSS pushed a commit that referenced this issue Aug 16, 2023
Refs: #48640
PR-URL: #48698
Fixes: #48858
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
RafaelGSS pushed a commit that referenced this issue Aug 17, 2023
Refs: #48640
PR-URL: #48698
Fixes: #48858
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
nodejs-github-bot pushed a commit that referenced this issue Oct 10, 2023
PR-URL: #48902
Fixes: #48640
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this issue Oct 28, 2023
Refs: #48640
PR-URL: #48698
Fixes: #48858
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
alexfernandez pushed a commit to alexfernandez/node that referenced this issue Nov 1, 2023
PR-URL: nodejs#48902
Fixes: nodejs#48640
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this issue Nov 11, 2023
PR-URL: #48902
Fixes: #48640
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
debanjum added a commit to khoj-ai/khoj that referenced this issue Apr 9, 2024
- `fs.readdir' func in node version 18.18.2 has buggy `recursive' option
  See nodejs/node#48640, Effect-TS/effect#1801 for details

- We were recursing down a folder in two ways on the Desktop app.
  Remove `recursive: True' option to the `fs.readdirSync' method call
  to recurse down via app code only
debanjum added a commit to khoj-ai/khoj that referenced this issue Apr 9, 2024
- `fs.readdir' func in node version 18.18.2 has buggy `recursive' option
  See nodejs/node#48640, Effect-TS/effect#1801 for details

- We were recursing down a folder in two ways on the Desktop app.
  Remove `recursive: True' option to the `fs.readdirSync' method call
  to recurse down via app code only
debanjum added a commit to khoj-ai/khoj that referenced this issue Apr 9, 2024
- `fs.readdir' func in node version 18.18.2 has buggy `recursive' option
  See nodejs/node#48640, Effect-TS/effect#1801 for details

- We were recursing down a folder in two ways on the Desktop app.
  Remove `recursive: True' option to the `fs.readdirSync' method call
  to recurse down via app code only
debadree25 pushed a commit to debadree25/node that referenced this issue Apr 15, 2024
PR-URL: nodejs#48902
Fixes: nodejs#48640
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
sercher added a commit to sercher/graaljs that referenced this issue Apr 25, 2024
Refs: nodejs/node#48640
PR-URL: nodejs/node#48698
Fixes: nodejs/node#48858
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
sercher added a commit to sercher/graaljs that referenced this issue Apr 25, 2024
Refs: nodejs/node#48640
PR-URL: nodejs/node#48698
Fixes: nodejs/node#48858
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants