search

paulmillr / readdirp

Recursive version of fs.readdir with streaming api.
https://paulmillr.com
MIT License
381 stars 52 forks source link

Support symlink on directories #151

Closed znarf closed 4 years ago

znarf commented 4 years ago

Fix https://github.com/paulmillr/readdirp/issues/150

paulmillr commented 4 years ago

We absolutely can’t use synchronous fs methods. They block event loop.

paulmillr commented 4 years ago

Ensure you add tests to the feature. Tests must fail before your commits and pass after.

znarf commented 4 years ago

@paullmilr

We absolutely can’t use synchronous fs methods. They block event loop.

Would you be fine if I refactor these parts using async/await ?

paulmillr commented 4 years ago

Sure.

Also, ensure the performance isn’t degrading by 50% or so. The library is used by millions of folks.

For example, the behavior should probably only be used for followSymlinks option

znarf commented 4 years ago

Also, ensure the performance isn’t degrading by 50% or so.

Do you have a benchmark to recommend?

For example, the behavior should probably only be used for followSymlinks option

Do you mean lstat: true? Or should we introduce a more explicit followSymlinks option?

Note: I've read the documentation but it's actually currently unclear to me what is the exact purpose of lstat: true, symlinks seems to be returned just fine without it.

paulmillr commented 4 years ago

node examples/bench.js

Try your case with lstat: true and see what happens.

znarf commented 4 years ago

node examples/bench.js

Cool, no regression but more files are being returned. Makes sense, because the symlinked directories are now followed.

Try your case with {lstat: true} and see what happens.

I tried and haven't seen a difference regarding symlinks. In the test suite (old test and new test), the symlinks are properly followed without setting this option. Maybe a relic of the past?

If you don't want the symlinks to be followed, I suggest to add an explicit followSymlinks as a boolean. Should I do that, default to true?

Except that, the PR is ready for review, refactored using async/await and added a test. The test was properly failing in CI, see https://github.com/paulmillr/readdirp/pull/152.

znarf commented 4 years ago

@paulmillr what do you think? could you review so we can move forward with that?

Some context about this change: I'm proposing to integrate readdirp in a nice other open source project and this issue is currently a blocker, ref https://github.com/depcheck/depcheck/pull/498

paulmillr commented 4 years ago

@znarf I will test this on my machine(s) and merge ASAP. Overall, looks good!

znarf commented 4 years ago

Any luck? :-)

paulmillr commented 4 years ago

Merged. Thank you. Required an adjustment to fix error when the result path doesn't exist.

znarf commented 4 years ago

@paulmillr Excellent! Thank you.