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

refactor: switch to native fs/promises #284

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SukkaW
Copy link

@SukkaW SukkaW commented Jul 12, 2023

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
Copy link
Author

SukkaW commented Jul 30, 2023

@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
Copy link
Owner

isaacs commented Jul 31, 2023

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
Copy link
Author

SukkaW commented Jul 31, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants