search

nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
108.08k stars 29.83k forks source link

fs.readdir(path, { recursive: true }) is sync #56006

Open RafaelGSS opened 1 week ago

RafaelGSS commented 1 week ago

Reviving https://github.com/nodejs/node/issues/51749.

The implementation in https://github.com/nodejs/node/pull/41439 is calling a synchronous approach even for an asynchronous call. See https://github.com/nodejs/node/blob/main/lib/fs.js#L1458.

I will take a quick look at that and see if I can fix it; otherwise, we might want to consider documenting it explicitly until we have a real solution for it.

cc: @Ethan-Arrowood

aduh95 commented 1 week ago

We could mark it as legacy and decide to not fix the issue, as long as the promise version is correctly async we can probably get away with the callback version being wrongly sync.

RafaelGSS commented 6 days ago

We could mark it as legacy and decide to not fix the issue, as long as the promise version is correctly async we can probably get away with the callback version being wrongly sync.

If a sync version has a feature and its callback equivalent call doesn't... looks like a bad UX to me. Have we considered a new function instead? It seems more appropriate: fs.recursiveReaddir()

RafaelGSS commented 6 days ago

Nevermind, I have a PR almost ready to fix this.