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 - option depth #49243

Closed
prettydiff opened this issue Aug 19, 2023 · 4 comments · May be fixed by #49255
Closed

fs.readdir - option depth #49243

prettydiff opened this issue Aug 19, 2023 · 4 comments · May be fixed by #49255
Labels
feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system. stale

Comments

@prettydiff
Copy link
Contributor

What is the problem this feature will solve?

In version 20.1.0 Node introduced new option recursive on fs.readdir. I needed this feature about 8 years ago so I wrote my own library to accomplish it, but I found that it can be exceptionally painful if you execute a recursive option as a boolean from too high in the file system tree.

To solve for in my own library I removed the recursive option and replaced it with an option named depth that takes a number. The floor value of that number determines how many rounds of recursion to apply when walking the file system.

  • If the value is less than 1 then I apply full recursion.
  • If the value is 1 then I apply no recursion.
  • If the value is 2 I gather file system contents of the specified directory and the immediate child directories.
  • Greater values then walk each additional descendent depth of the file system per the supplied value.

An example of where this is helpful is applying a depth 2 on Windows from "C:". Will full recursion it will just fail.

What is the feature you are proposing to solve the problem?

I am proposing an option named depth for fs.readdir. I am not sure, but it looks like would be executed at:

https://github.com/nodejs/node/blob/main/lib/fs.js#L1427

Here is how I am calculating depth in my own code: filePath.replace(startItem, "").split(vars.path.sep).length

https://github.com/prettydiff/share-file-systems/blob/master/lib/terminal/commands/library/directory.ts#L409

With a bit of guidance I will happily do the work and submit a pull request. I just need:

  1. Am I looking at the correct location in the Node code base?
  2. If the current way I solve for this in my own code is less desirable I will write the solution in a different way.
  3. I would need guidance to build Node so that I can test that my enhancement works correctly.
  4. I would need guidance to supply and execute unit tests however Node executes unit tests.

What alternatives have you considered?

As an alternative I wrote my own solution about 8 years ago.

@prettydiff prettydiff added the feature request Issues that request new features to be added to Node.js. label Aug 19, 2023
@aduh95
Copy link
Contributor

aduh95 commented Aug 19, 2023

  1. Am I looking at the correct location in the Node code base?

You linked to the sync implementation, we would probably want to update the async APIs (callback and promise) as well. IMO a good strategy would be to open a draft PR touching only the sync API (and preferably adding tests, that's the best way to show off how the feature is supposed to work) – that should let folks the opportunity to send early feedback on the feature before you spend a lot of time on the other APIs.

2. If the current way I solve for this in my own code is less desirable I will write the solution in a different way.

This can be discussed in the PR as code review, it seems a bit hard to discuss in the abstract.

3. I would need guidance to build Node so that I can test that my enhancement works correctly.

This is documented in BUILDING.md, don't hesitate to open an issue if you see something's missing. You can also ask on #nodejs-core on the OpenJS Foundation Slack.

4. I would need guidance to supply and execute unit tests however Node executes unit tests.

This is documented in in test/README.md, same advice applies. My advice would be to start from test/sequential/test-fs-readdir-recursive.js and adapt it to the API you're implementing.

@prettydiff
Copy link
Contributor Author

#49255

Copy link
Contributor

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Feb 19, 2024
Copy link
Contributor

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system. stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants