-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
fs: add recursive opendir/readdir #41439
Changes from all commits
1e6c6f6
a261523
0864860
0d6fac0
7a52f25
151ad03
cbd7033
46919a2
48372d5
2fa3aa7
b7f403e
a3e55da
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -1404,6 +1404,36 @@ function mkdirSync(path, options) { | |||||||
} | ||||||||
} | ||||||||
|
||||||||
// TODO(Ethan-Arrowood): Make this iterative too | ||||||||
function readdirSyncRecursive(path, origPath, options) { | ||||||||
nullCheck(path, 'path', true); | ||||||||
const ctx = { path }; | ||||||||
const result = binding.readdir(pathModule.toNamespacedPath(path), | ||||||||
options.encoding, !!options.withFileTypes, undefined, ctx); | ||||||||
handleErrorFromBinding(ctx); | ||||||||
return options.withFileTypes ? | ||||||||
getDirents(path, result).flatMap((dirent) => { | ||||||||
return [ | ||||||||
dirent, | ||||||||
...(dirent.isDirectory() ? | ||||||||
readdirSyncRecursive( | ||||||||
pathModule.join(path, dirent.name), | ||||||||
origPath, | ||||||||
options, | ||||||||
) : []), | ||||||||
]; | ||||||||
}) : | ||||||||
result.flatMap((ent) => { | ||||||||
const innerPath = pathModule.join(path, ent); | ||||||||
const relativePath = pathModule.relative(origPath, innerPath); | ||||||||
const stat = binding.internalModuleStat(innerPath); | ||||||||
return [ | ||||||||
relativePath, | ||||||||
...(stat === 1 ? readdirSyncRecursive(innerPath, origPath, options) : []), | ||||||||
]; | ||||||||
}); | ||||||||
} | ||||||||
|
||||||||
/** | ||||||||
* Reads the contents of a directory. | ||||||||
* @param {string | Buffer | URL} path | ||||||||
|
@@ -1421,6 +1451,14 @@ function readdir(path, options, callback) { | |||||||
callback = makeCallback(typeof options === 'function' ? options : callback); | ||||||||
options = getOptions(options); | ||||||||
path = getValidatedPath(path); | ||||||||
if (options.recursive != null) { | ||||||||
validateBoolean(options.recursive, 'options.recursive'); | ||||||||
} | ||||||||
|
||||||||
if (options.recursive) { | ||||||||
callback(null, readdirSyncRecursive(path, path, options)); | ||||||||
return; | ||||||||
} | ||||||||
|
||||||||
const req = new FSReqCallback(); | ||||||||
if (!options.withFileTypes) { | ||||||||
|
@@ -1444,12 +1482,21 @@ function readdir(path, options, callback) { | |||||||
* @param {string | { | ||||||||
* encoding?: string; | ||||||||
* withFileTypes?: boolean; | ||||||||
* recursive?: boolean; | ||||||||
* }} [options] | ||||||||
* @returns {string | Buffer[] | Dirent[]} | ||||||||
*/ | ||||||||
function readdirSync(path, options) { | ||||||||
options = getOptions(options); | ||||||||
path = getValidatedPath(path); | ||||||||
if (options.recursive != null) { | ||||||||
validateBoolean(options.recursive, 'options.recursive'); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have to be strict about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not being strict with boolean type checks create issues where you also have to check There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I follow. Loose typecheck on positional arguments can be error-prone in some scenarios, but I don't see an issue in this form. Can you provide an example? Lines 1496 to 1498 in 611a616
If strict check is preferable, perhaps we shouldn't allow null value either, and align other boolean parameters with it.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. I recommend @Ethan-Arrowood to follow up with this on a different pull request, if it's ok with you? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a part of stable API, so changing it separately might be That said, I'm not blocking this PR in current form, but it would be nice to have consensus on what we want to have after follow-ups before landing this one. |
||||||||
} | ||||||||
|
||||||||
if (options.recursive) { | ||||||||
return readdirSyncRecursive(path, path, options); | ||||||||
} | ||||||||
|
||||||||
const ctx = { path }; | ||||||||
const result = binding.readdir(pathModule.toNamespacedPath(path), | ||||||||
options.encoding, !!options.withFileTypes, | ||||||||
|
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 probably applies to all documented APIs?