-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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: prefer native fs.promises #3346
Conversation
@@ -139,6 +139,14 @@ t.test('completion to folder', async t => { | |||
else | |||
return ['package.json'] | |||
}, | |||
promises: { | |||
readdir: (path) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The environment without fs.promises
has already been tested here:
cli/test/lib/utils/reify-finish.js
Lines 80 to 86 in 73633e5
t.test('works without fs.promises', async t => { | |
t.doesNotThrow(() => t.mock('../../../lib/utils/reify-finish.js', { | |
fs: { ...fs, promises: null }, | |
'../../../lib/npm.js': npm, | |
'../../../lib/utils/reify-output.js': reifyOutput, | |
})) | |
}) |
@@ -4,7 +4,7 @@ const rimraf = util.promisify(require('rimraf')) | |||
const reifyFinish = require('./utils/reify-finish.js') | |||
const runScript = require('@npmcli/run-script') | |||
const fs = require('fs') | |||
const readdir = util.promisify(fs.readdir) | |||
const readdir = fs.promises.readdir || util.promisify(fs.readdir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will throw when fs.promises is undefined.
Until we drop support for node versions lacking fs.promises, it's simpler and safer to just use promisify. It avoids more complexity than it introduces, and there is no significant benefit to using fs.promises directly. We'll do this refactor for npm v8, to use const {
readdir,
readFile,
stat,
} = require('fs').promises is certainly a heck of a lot nicer to look at than: const { promisify } = require('util')
const fs = require('fs')
const readdir = promisify(fs.readdir)
const readFile = promisify(fs.readFile)
const stat = promisify(fs.stat) |
Prefer native built-in
fs.promises
, then fallback toutil.promisfy
.References
See also #2654