Begin docs-deprecation of `fs.readdir{Sync}()`, add `fs.scandirSync()` alias. #36008

Fishrock123 · 2020-11-06T21:10:07Z

It's time overdue for these to start going away.

fs.opendir() was introduced over a year ago in #29349 - which stated a follow-up of: "Aliasing fs.readdir to fs.scandir and doing a deprecation."

This provides the intial docs deprecation to both fs.readdir() and fs.readdirSync(), both of which are misnamed, and the former of which is a trap as it is not fully async.

This also contains an addition of fs.scandirSync(), also a follow-up to #29349 and a precursor to deprecating both fs.readdir() and fs.readdirSync().

This also updates the documentation which has been incorrect for some 10 years saying that these did readdir(3) when in reality they do scandir(3).

Only scandirSync() is introduced as "async scandir(3)" is kind of a trap, given that it returns the whole list of entries at once, regardless of how many there are. Since in many cases we'd also want to get dirents for them (i.e. stat-ing each and every one), this becomes a serious problem, and Node.js should encourage users to use fs.opendir(), which is slightly more complex but far better.

A follow-up thought: Regarding a potential fs.scandir(), should people think it is in some way desirable for api parity, I believe it should be deprecated out of the gate, or warned against heavily in the docs.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

This is a follow-up to nodejs#29349 and a precursor to deprecating both `fs.readdir()` and `fs.readdirSync()`. This also updates the documentation which has been incorrect for some 10 years saying that these did `readdir(3)` when in reality they do `scandir(3)`. Only `scandirSync()` is introduced as "async scandir(3)" is kind of a trap, given that it returns the whole list of entries at once, regardless of how many there are. Since in many cases we'd also want to get dirents for them (i.e. `stat`-ing each and every one), this becomes a serious problem, and Node.js should encourage users to use `fs.opendir()`, which is slightly more complex but far better.

It's time overdue for these to start going away. `fs.opendir()` was introduced over a year ago in nodejs#29349 - which stated a follow-up of: "Aliasing fs.readdir to fs.scandir and doing a deprecation." This provides the intial docs deprecation to both `fs.readdir()` and `fs.readdirSync()`, both of which are misnamed, and the former of which is a trap as it is not fully async.

aduh95 · 2020-11-06T22:04:42Z

doc/api/deprecations.md

+<!-- YAML
+changes:
+  - version: REPLACEME
+    pr-url: REPLACEME


Suggested change

pr-url: REPLACEME

pr-url: https://github.com/nodejs/node/pull/36008

aduh95 · 2020-11-06T22:05:10Z

doc/api/deprecations.md

+where possible, otherwise use [`fs.opendir()`][].
+`fs.readdir()` is not fully asynchronous. It is asynchronous while gathering
+the list of directory entries but then sends them to JavaScript as one large
+array. In cases where this array contains hndreds of entries, particularly if


Suggested change

array. In cases where this array contains hndreds of entries, particularly if

array. In cases where this array contains hundreds of entries, particularly if

aduh95 · 2020-11-06T22:05:49Z

doc/api/deprecations.md

@@ -2750,9 +2781,14 @@ runtime warning results no matter where the `"exports"` usage occurs.
 [`fs.lchmodSync(path, mode)`]: fs.md#fs_fs_lchmodsync_path_mode
 [`fs.lchown(path, uid, gid, callback)`]: fs.md#fs_fs_lchown_path_uid_gid_callback
 [`fs.lchownSync(path, uid, gid)`]: fs.md#fs_fs_lchownsync_path_uid_gid
+[`fs.opendir()`]: fs.html#fs_fs_opendir_path_options_callback


Suggested change

[`fs.opendir()`]: fs.html#fs_fs_opendir_path_options_callback

[`fs.opendir()`]: fs.md#fs_fs_opendir_path_options_callback

aduh95 · 2020-11-06T22:06:32Z

doc/api/fs.md

@@ -6125,6 +6152,7 @@ the file contents.
 [`fs.readv()`]: #fs_fs_readv_fd_buffers_position_callback
 [`fs.realpath()`]: #fs_fs_realpath_path_options_callback
 [`fs.rmdir()`]: #fs_fs_rmdir_path_options_callback
+[`fs.scandirSync()`]: fs.html#fs_fs_scandirsync_path_options


Suggested change

[`fs.scandirSync()`]: fs.html#fs_fs_scandirsync_path_options

[`fs.scandirSync()`]: #fs_fs_scandirsync_path_options

aduh95 · 2020-11-06T22:08:14Z

doc/api/fs.md

@@ -3011,13 +3015,15 @@ changes:
                 protocol. Support is currently still *experimental*.
 -->

+> Stability: 0 - Deprecated. Use [`fs.scandirSync()`][] instead.


There shoud be a deprecated: REPLACEME in the YAML above.

addaleax

I would prefer not to do this, apart from explicitly recommending fs.promises.opendir()…

I don’t think readdir() is misnamed enough to justify deprecating it and adding it under a different name (and in my subjective opionion, readdir() is a clearer name than scandir() even), and I also don’t think that using fs.readdir() is inherently a bad thing to use, as the case of directories with more than a few dozen entries is extremely rare in practice.

addaleax · 2020-11-07T20:55:56Z

doc/api/deprecations.md

+
+[`fs.readdir()`][] is deprecated. Use [`fsPromises.opendir()`][] instead,
+where possible, otherwise use [`fs.opendir()`][].
+`fs.readdir()` is not fully asynchronous. It is asynchronous while gathering


How is fs.readdir() not fully asynchronous? That seems inaccurate

github-actions · 2020-12-29T15:41:39Z

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

github-actions · 2021-01-29T00:06:41Z

Closing this because it has stalled. Feel free to reopen if this PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.

Fishrock123 added 2 commits November 6, 2020 13:03

Fishrock123 added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. semver-minor PRs that contain new features and should be released in the next minor version. labels Nov 6, 2020

Fishrock123 requested a review from Trott November 6, 2020 21:10

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 6, 2020

aduh95 reviewed Nov 6, 2020

View reviewed changes

addaleax requested changes Nov 7, 2020

View reviewed changes

addaleax added the deprecations Issues and PRs related to deprecations. label Nov 7, 2020

aduh95 added the stalled Issues and PRs that are stalled. label Dec 29, 2020

github-actions bot closed this Jan 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Begin docs-deprecation of `fs.readdir{Sync}()`, add `fs.scandirSync()` alias. #36008

Begin docs-deprecation of `fs.readdir{Sync}()`, add `fs.scandirSync()` alias. #36008

Fishrock123 commented Nov 6, 2020 •

edited

Loading

aduh95 Nov 6, 2020

aduh95 Nov 6, 2020

aduh95 Nov 6, 2020

aduh95 Nov 6, 2020

aduh95 Nov 6, 2020

addaleax left a comment

addaleax Nov 7, 2020

github-actions bot commented Dec 29, 2020

github-actions bot commented Jan 29, 2021

	pr-url: REPLACEME
	pr-url: https://github.com/nodejs/node/pull/36008

	array. In cases where this array contains hndreds of entries, particularly if
	array. In cases where this array contains hundreds of entries, particularly if

	[`fs.opendir()`]: fs.html#fs_fs_opendir_path_options_callback
	[`fs.opendir()`]: fs.md#fs_fs_opendir_path_options_callback

	[`fs.scandirSync()`]: fs.html#fs_fs_scandirsync_path_options
	[`fs.scandirSync()`]: #fs_fs_scandirsync_path_options

Begin docs-deprecation of fs.readdir{Sync}(), add fs.scandirSync() alias. #36008

Begin docs-deprecation of fs.readdir{Sync}(), add fs.scandirSync() alias. #36008

Conversation

Fishrock123 commented Nov 6, 2020 • edited Loading

Checklist

aduh95 Nov 6, 2020

Choose a reason for hiding this comment

aduh95 Nov 6, 2020

Choose a reason for hiding this comment

aduh95 Nov 6, 2020

Choose a reason for hiding this comment

aduh95 Nov 6, 2020

Choose a reason for hiding this comment

aduh95 Nov 6, 2020

Choose a reason for hiding this comment

addaleax left a comment

Choose a reason for hiding this comment

addaleax Nov 7, 2020

Choose a reason for hiding this comment

github-actions bot commented Dec 29, 2020

github-actions bot commented Jan 29, 2021

Begin docs-deprecation of `fs.readdir{Sync}()`, add `fs.scandirSync()` alias. #36008

Begin docs-deprecation of `fs.readdir{Sync}()`, add `fs.scandirSync()` alias. #36008

Fishrock123 commented Nov 6, 2020 •

edited

Loading