fs: move fs.promises API to fs/promises #18777

targos · 2018-02-14T09:46:34Z

First commit moves utility functions that both fs and fs.promises make use of to internal/fs. It will make backporting easier if needed.

Second commit moves fs.promises to a separate file: fs/promises. It also changes the async function definitions to regular declarations. This allowed to find a bug thanks to ESLint.

I'm doing this for three reasons:

fs was already a large file before the addition of fs.promises. This lightens it a bit.
fs.promises code is independent of fs code. It should not have to be loaded at the same time.
a separate module will allow to use named imports in ESM for promises functions.

/cc @jasnell @joyeecheung

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

Affected core subsystem(s)

fs

targos · 2018-02-14T09:47:13Z

lib/fs.js

-    if (constants.O_SYMLINK !== undefined) {
-      const fd = await promises.open(path,
-                                     constants.O_WRONLY | constants.O_SYMLINK);
-      return promises.fschmod(fd, mode).finally(fd.close.bind(fd));


this is the bug caught by moving out of an object

@targos mind explaining why catching the bug required moving it out of an object?

I realize that eslint has a much harder time to figure out missing properties on objects than ReferenceErrors - but not catching such typos is kind of embarrassing for us - more so if this wasn't a stage 1 API.

AFAIK eslint cannot check properties on objects. It is not something that can be easily done statically.
The best way to catch such errors is to increase code coverage.

targos · 2018-02-14T09:47:32Z

lib/fs.js

-    if (constants.O_SYMLINK !== undefined) {
-      const fd = await promises.open(path,
-                                     constants.O_WRONLY | constants.O_SYMLINK);
-      return promises.fschmod(fd, uid, gid).finally(fd.close.bind(fd));


benjamingr

Overall bug fix and code movement lgtm.

benjamingr · 2018-02-14T12:37:10Z

doc/api/fs.md

@@ -3231,11 +3231,11 @@ in that, if the `FileHandle` is not explicitly closed using the
 and will emit a process warning, thereby helping to prevent memory leaks.

 Instances of the `FileHandle` object are created internally by the
-`fs.promises.open()` method.
+`fsPromises.open()` method.


I'd prefer it if it was referred to as simply fs in the examples (where it's clear it's the promise version) - that's because that's how people typically use it. Not blocking on it.

I also prefer to use just fs. Let's wait and see if someone disagrees.

I prefer fsPromises.something because that is clearer...if we use fs.something then it looks confusing for new comers if they don't read through the document and notice that they need to get the promisified version that way.

EDIT: s/fs.promise/fsPromises/

After checking how the documentation is, I changed my mind and also prefer to use a different name.
Ideally, I think it should be possible to switch from the callback documentation to the Promise one instead of having one giant page with both at the same time.

benjamingr · 2018-02-14T12:39:45Z

lib/fs.js

-    if (constants.O_SYMLINK !== undefined) {
-      const fd = await promises.open(path,
-                                     constants.O_WRONLY | constants.O_SYMLINK);
-      return promises.fschmod(fd, mode).finally(fd.close.bind(fd));


@targos mind explaining why catching the bug required moving it out of an object?

I realize that eslint has a much harder time to figure out missing properties on objects than ReferenceErrors - but not catching such typos is kind of embarrassing for us - more so if this wasn't a stage 1 API.

benjamingr · 2018-02-14T12:40:43Z

lib/internal/fs.js

 } = process.binding('constants').fs;
+const { statValues } = process.binding('fs');
+
+const isWindows = process.platform === 'win32';


Not that it matters, but we repeat this line in quite a few places.

benjamingr · 2018-02-14T12:41:34Z

lib/internal/fs.js


 function assertEncoding(encoding) {
  if (encoding && !Buffer.isEncoding(encoding)) {
    throw new errors.TypeError('ERR_INVALID_OPT_VALUE_ENCODING', encoding);
  }
 }

+function copyObject(source) {


I know this PR just moves this - but do you know the rationale for reimplementing this over?

Then I guess I'll just follow up on it after this lands

benjamingr · 2018-02-14T12:45:12Z

I'm +0 on require(fs/promises) vs require(fs).promises - but I'm +1 on splitting the file away anyway.

We're still at a point where moving it isn't a big deal. Also ping @jasnell.

joyeecheung

Thanks for doing this!

targos · 2018-02-16T14:50:11Z

CI: https://ci.nodejs.org/job/node-test-pull-request/13202/
Edit: again: https://ci.nodejs.org/job/node-test-pull-request/13215/

devsnek · 2018-02-16T14:54:17Z

lib/fs/promises.js

+    return fsync(this);
+  }
+
+


extra space?

targos · 2018-02-17T09:23:09Z

Landed in 760f86c...513d939

PR-URL: #18777 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

ljharb · 2018-05-02T06:48:50Z

imo this should not have been merged unflagged, pending the outcome of nodejs/TSC#389 (comment) (specifically, making fs/promises exposed unflagged)

PR-URL: nodejs#18777 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

nodejs-github-bot added build Issues and PRs related to build files or the CI. fs Issues and PRs related to the fs subsystem / file system. labels Feb 14, 2018

targos commented Feb 14, 2018

View reviewed changes

targos added promises Issues and PRs related to ECMAScript promises. and removed build Issues and PRs related to build files or the CI. labels Feb 14, 2018

benjamingr approved these changes Feb 14, 2018

View reviewed changes

devsnek approved these changes Feb 14, 2018

View reviewed changes

jasnell approved these changes Feb 14, 2018

View reviewed changes

benjamingr mentioned this pull request Feb 15, 2018

src: fix the returned Promise of fs.promises.symlink #18791

Closed

2 tasks

joyeecheung approved these changes Feb 16, 2018

View reviewed changes

targos added 2 commits February 16, 2018 15:45

fs: move utility functions to internal/fs

fab7afe

fs: move fs.promises API to fs/promises

5df426f

targos force-pushed the fs-promises branch from d3a57e7 to 5df426f Compare February 16, 2018 14:48

targos added dont-land-on-v4.x labels Feb 16, 2018

devsnek reviewed Feb 16, 2018

View reviewed changes

lib/fs/promises.js Outdated

return fsync(this);

}

Copy link

Member

devsnek Feb 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra space?

fixup: remove extra space

75d27f1

targos added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 16, 2018

targos closed this Feb 17, 2018

targos deleted the fs-promises branch February 17, 2018 09:23

RyanZim mentioned this pull request Apr 3, 2018

Node.js 10 jprichardson/node-fs-extra#551

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: move fs.promises API to fs/promises #18777

fs: move fs.promises API to fs/promises #18777

targos commented Feb 14, 2018

targos Feb 14, 2018

benjamingr Feb 14, 2018 •

edited

Loading

targos Feb 14, 2018

targos Feb 14, 2018

benjamingr left a comment

benjamingr Feb 14, 2018

targos Feb 14, 2018

joyeecheung Feb 16, 2018 •

edited

Loading

joyeecheung Feb 16, 2018 •

edited

Loading

targos Feb 16, 2018

benjamingr Feb 14, 2018 •

edited

Loading

benjamingr Feb 14, 2018

benjamingr Feb 14, 2018

targos Feb 14, 2018

benjamingr Feb 14, 2018

benjamingr commented Feb 14, 2018 •

edited

Loading

joyeecheung left a comment

targos commented Feb 16, 2018 •

edited

Loading

devsnek Feb 16, 2018

targos commented Feb 17, 2018

ljharb commented May 2, 2018 •

edited

Loading

fs: move fs.promises API to fs/promises #18777

fs: move fs.promises API to fs/promises #18777

Conversation

targos commented Feb 14, 2018

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

benjamingr Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamingr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

joyeecheung Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamingr Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamingr commented Feb 14, 2018 • edited Loading

joyeecheung left a comment

Choose a reason for hiding this comment

targos commented Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

targos commented Feb 17, 2018

ljharb commented May 2, 2018 • edited Loading

benjamingr Feb 14, 2018 •

edited

Loading

joyeecheung Feb 16, 2018 •

edited

Loading

joyeecheung Feb 16, 2018 •

edited

Loading

benjamingr Feb 14, 2018 •

edited

Loading

benjamingr commented Feb 14, 2018 •

edited

Loading

targos commented Feb 16, 2018 •

edited

Loading

ljharb commented May 2, 2018 •

edited

Loading