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

fs: promisify exists correctly #13316

Closed
wants to merge 1 commit into from

Conversation

dfabulich
Copy link
Contributor

@dfabulich dfabulich commented May 30, 2017

Consider this sample code.

const fs = require('fs');
const {promisify} = require('util');

const exists = promisify(fs.exists);

exists(__filename)
  .then(x => console.log(x))
  .catch(err => console.error("oh no", err));

Expected: The code should log true, because the current file exists.
Actual: It logs oh no true, as the promise is rejected with err set to true

This is happening because fs.exists does not follow the standard format of taking an error-first callback as the last argument. Luckily, util.promisify provides a .custom feature for promisifying callbacks that don't follow the nodeback style.

In this PR, we provide a custom promise implementation for fs.exists.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

fs

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label May 30, 2017
@@ -29,3 +30,5 @@ common.refreshTmpDir();
fs.closeSync(fd);
}));
}

exists(__filename).then((x) => assert(x));
Copy link
Member

Choose a reason for hiding this comment

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

I’d recommend wrapping these in {}, too (even if it’s just a single line), and the callback should probably be wrapped in common.mustCall()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

lib/fs.js Outdated
@@ -376,6 +376,13 @@ fs.exists = function(path, callback) {
}
};

Object.defineProperty(fs.exists, internalUtil.promisify.custom, {
value: (path) => new Promise((resolve) =>
fs.exists(path, (x) => resolve(x))),
Copy link
Member

Choose a reason for hiding this comment

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

You could just use resolve instead of (x) => resolve(x), if you like

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@addaleax addaleax added promises Issues and PRs related to ECMAScript promises. semver-minor PRs that contain new features and should be released in the next minor version. labels May 30, 2017
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

LGTM % 2 nits

@@ -29,3 +30,7 @@ common.refreshTmpDir();
fs.closeSync(fd);
}));
}

{
exists(__filename).then(common.mustCall((x) => assert(x)));
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO it's more important to assert:
assert.strictEqual(typeof x, 'boolean')

Copy link
Member

Choose a reason for hiding this comment

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

In that case you might just strictEqual(x, true) ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, assert.strictEqual(x, true);

lib/fs.js Outdated
@@ -376,6 +376,12 @@ fs.exists = function(path, callback) {
}
};

Object.defineProperty(fs.exists, internalUtil.promisify.custom, {
value: (path) => new Promise((resolve) => fs.exists(path, resolve)),
Copy link
Contributor

Choose a reason for hiding this comment

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

very small nit: place the enumerable: false before the value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

meh, done

Copy link
Contributor

Choose a reason for hiding this comment

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

🤷‍♂️ it's a style preference.
Thank you for indulging me 🎩

lib/fs.js Outdated
@@ -376,6 +376,12 @@ fs.exists = function(path, callback) {
}
};

Object.defineProperty(fs.exists, internalUtil.promisify.custom, {
enumerable: false,
value: (path) => new Promise((resolve) => fs.exists(path, resolve))
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the whole point of promisify NOT to use new Promise?

Copy link
Member

Choose a reason for hiding this comment

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

No, the point of promisify is to give users a standard interface for turning Node-style callbacks into functions that return Promises. And since this is a deprecated API, I don’t think there’s a need to micro-optimize.

Copy link
Contributor Author

@dfabulich dfabulich May 30, 2017

Choose a reason for hiding this comment

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

meh, done. It's now fully micro-optimized 😉

lib/fs.js Outdated
@@ -376,6 +376,16 @@ fs.exists = function(path, callback) {
}
};

Object.defineProperty(fs.exists, internalUtil.promisify.custom, {
enumerable: false,

Choose a reason for hiding this comment

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

All values in defineProperty default to false, so this may not be necessary?

Alternatively, is it intentional that it is not configurable or writable? (I assume at least the latter is true)

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK it's supposed to be "hidden" from userland.
See lib/fs.js#L780

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, I was copying and pasting from line 780, because I incorrectly assumed that enumerable: false was required to conceal it from userland.

MDN says the defaults are:

{enumerable: false, configurable: false, writeable: false}

Those look like good defaults to me. I'll remove enumerable: false here and use the defaults. (I guess we should remove it at line 780, but I'll let somebody else do that.)

Copy link
Member

Choose a reason for hiding this comment

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

I like being explicit in code, especially if it helps clarify intent and when the defaults are not obvious to casual readers, which is why I added the enumerable: false; but I won’t force that on you if you don’t like it.

(@dfabulich Thanks for being so quick with updating here, but just so there’s no confusion: Our opinions aren’t inherently better than yours, and if you think a collaborator makes a request that you don’t agree with, you should feel free to say so.)

Copy link
Contributor Author

@dfabulich dfabulich May 31, 2017

Choose a reason for hiding this comment

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

I think all of the proposed changes here have been (small) improvements or no difference to me. I aim to please. :-)

I do think it's about ready to land now.

@jasnell
Copy link
Member

jasnell commented Jun 1, 2017

jasnell pushed a commit that referenced this pull request Jun 1, 2017
PR-URL: #13316
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Vladimir Kurchatkin <vladimir.kurchatkin@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
@vsemozhetbyt
Copy link
Contributor

It seems this can be closed now with 65531d0 ?

@jasnell
Copy link
Member

jasnell commented Jun 1, 2017

Yes, was just getting to that ;-) ... Landed in 65531d0

@jasnell jasnell closed this Jun 1, 2017
jasnell pushed a commit that referenced this pull request Jun 5, 2017
PR-URL: #13316
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Vladimir Kurchatkin <vladimir.kurchatkin@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
@jasnell jasnell mentioned this pull request Jun 5, 2017
jasnell added a commit to jasnell/node that referenced this pull request Jun 7, 2017
* **Async Hooks**
  * When one `Promise` leads to the creation of a new `Promise`, the parent
    `Promise` will be identified as the trigger
    [[`135f4e6643`](nodejs@135f4e6643)]
    [nodejs#13367](nodejs#13367).
* **Dependencies**
  * libuv has been updated to 1.12.0
    [[`968596ec77`](nodejs@968596ec77)]
    [nodejs#13306](nodejs#13306).
  * npm has been updated to 5.0.3
    [[`ffa7debd7a`](nodejs@ffa7debd7a)]
    [nodejs#13487](nodejs#13487).
* **File system**
  * The `fs.exists()` function now works correctly with `util.promisify()`
    [[`6e0eccd7a1`](nodejs@6e0eccd7a1)]
    [nodejs#13316](nodejs#13316).
  * fs.Stats times are now also available as numbers
    [[`c756efb25a`](nodejs@c756efb25a)]
    [nodejs#13173](nodejs#13173).
* **Inspector**
  * It is now possible to bind to a random port using `--inspect=0`
    [[`cc6ec2fb27`](nodejs@cc6ec2fb27)]
    [nodejs#5025](nodejs#5025).
* **Zlib**
  * A regression in the Zlib module that made it impossible to properly
    subclasses `zlib.Deflate` and other Zlib classes has been fixed.
    [[`6aeb555cc4`](nodejs@6aeb555cc4)]
    [nodejs#13374](nodejs#13374).
rvagg pushed a commit that referenced this pull request Jun 8, 2017
* **Async Hooks**
  * When one `Promise` leads to the creation of a new `Promise`, the parent
    `Promise` will be identified as the trigger
    [[`135f4e6643`](135f4e6643)]
    [#13367](#13367).
* **Dependencies**
  * libuv has been updated to 1.12.0
    [[`968596ec77`](968596ec77)]
    [#13306](#13306).
  * npm has been updated to 5.0.3
    [[`ffa7debd7a`](ffa7debd7a)]
    [#13487](#13487).
* **File system**
  * The `fs.exists()` function now works correctly with `util.promisify()`
    [[`6e0eccd7a1`](6e0eccd7a1)]
    [#13316](#13316).
  * fs.Stats times are now also available as numbers
    [[`c756efb25a`](c756efb25a)]
    [#13173](#13173).
* **Inspector**
  * It is now possible to bind to a random port using `--inspect=0`
    [[`cc6ec2fb27`](cc6ec2fb27)]
    [#5025](#5025).
* **Zlib**
  * A regression in the Zlib module that made it impossible to properly
    subclasses `zlib.Deflate` and other Zlib classes has been fixed.
    [[`6aeb555cc4`](6aeb555cc4)]
    [#13374](#13374).
rvagg pushed a commit that referenced this pull request Jun 8, 2017
* **Async Hooks**
  * When one `Promise` leads to the creation of a new `Promise`, the parent
    `Promise` will be identified as the trigger
    [[`135f4e6643`](135f4e6643)]
    [#13367](#13367).
* **Dependencies**
  * libuv has been updated to 1.12.0
    [[`968596ec77`](968596ec77)]
    [#13306](#13306).
  * npm has been updated to 5.0.3
    [[`ffa7debd7a`](ffa7debd7a)]
    [#13487](#13487).
* **File system**
  * The `fs.exists()` function now works correctly with `util.promisify()`
    [[`6e0eccd7a1`](6e0eccd7a1)]
    [#13316](#13316).
  * fs.Stats times are now also available as numbers
    [[`c756efb25a`](c756efb25a)]
    [#13173](#13173).
* **Inspector**
  * It is now possible to bind to a random port using `--inspect=0`
    [[`cc6ec2fb27`](cc6ec2fb27)]
    [#5025](#5025).
* **Zlib**
  * A regression in the Zlib module that made it impossible to properly
    subclasses `zlib.Deflate` and other Zlib classes has been fixed.
    [[`6aeb555cc4`](6aeb555cc4)]
    [#13374](#13374).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. promises Issues and PRs related to ECMAScript promises. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.