search

isaacs / rimraf

A `rm -rf` util for nodejs
ISC License
5.62k stars 250 forks source link

refactor: switch to native `fs/promises` #284

Open SukkaW opened 1 year ago

SukkaW commented 1 year ago

The PR replaces home-baked promisified fs methods with the native fs/promises.

rimraf is already targeting Node.js 14, and the fs/promises API has been available since Node.js 14.0.0:

image

https://nodejs.org/dist/latest-v14.x/docs/api/fs.html#promises-api

The compatibility can be tested with CI.

SukkaW commented 1 year ago

@isaacs Kindly request your attention with a friendly ping. My PR has been open for 3 weeks. I would greatly appreciate it if you could spare some time to review it, at your convenience, and provide your valuable feedback.

isaacs commented 1 year ago

Sorry for the delay. Whole family got covid. I'm traveling this week, I'll take a closer look when I get back.

The reason these methods are the way they are was originally too eek out a bit of extra performance, because manually promisifying in this way was faster than the built in promisified fs/promises implementation. I'll have to check the benchmarks to see if that's still the case before landing.

SukkaW commented 1 year ago

Sorry for the delay. Whole family got covid. I'm traveling this week, I'll take a closer look when I get back.

I hope for a speedy recovery for your family!

The reason these methods are the way they are was originally too eek out a bit of extra performance, because manually promisifying in this way was faster than the built in promisified fs/promises implementation. I'll have to check the benchmarks to see if that's still the case before landing.

Make sense. And here is my initial benchmark result from my Intel MacBook Pro, Node.js 18.16.0 only.

Node.js 18.16.0 main image
Node.js 18.16.0 SukkaW:native-fs.promises image