fs: promisify exists correctly by dfabulich · Pull Request #13316 · nodejs/node

dfabulich · 2017-05-30T21:45:12Z

Consider this sample code.

constfs=require('fs');const{promisify}=require('util');constexists=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

addaleax · 2017-05-30T21:46:36Z

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()

addaleax · 2017-05-30T21:47:14Z

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

refack

LGTM % 2 nits

refack · 2017-05-30T22:11:17Z

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

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

done, assert.strictEqual(x, true);

refack · 2017-05-30T22:11:33Z

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

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

vkurchatkin · 2017-05-30T22:55:03Z

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

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.

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

Jessidhia · 2017-05-31T05:49:40Z

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)

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

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.)

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.)

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 · 2017-06-01T20:35:56Z

CI: https://ci.nodejs.org/job/node-test-pull-request/8412/

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 · 2017-06-01T22:33:00Z

It seems this can be closed now with 65531d0 ?

jasnell · 2017-06-01T22:34:53Z

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

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>

* **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).

* **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).

nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label May 30, 2017

addaleax approved these changes May 30, 2017
View reviewed changes

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

dfabulich force-pushed the dgf-exists-promisify branch from 092634c to e3d3f3eCompare May 30, 2017 21:52

refack approved these changes May 30, 2017
View reviewed changes

dfabulich force-pushed the dgf-exists-promisify branch from e3d3f3e to 171a9aaCompare May 30, 2017 22:17

refack approved these changes May 30, 2017
View reviewed changes

jasnell approved these changes May 30, 2017
View reviewed changes

vkurchatkin suggested changes May 30, 2017
View reviewed changes

dfabulich force-pushed the dgf-exists-promisify branch from 171a9aa to 8d93e74Compare May 30, 2017 23:12

vkurchatkin approved these changes May 30, 2017
View reviewed changes

lpinca approved these changes May 31, 2017
View reviewed changes

Jessidhia reviewed May 31, 2017
View reviewed changes

fs: promisify exists correctly
15f3f0b

dfabulich force-pushed the dgf-exists-promisify branch from 8d93e74 to 15f3f0bCompare May 31, 2017 16:52

TimothyGu approved these changes May 31, 2017
View reviewed changes

jasnell closed this Jun 1, 2017

jasnell mentioned this pull request Jun 5, 2017
8.1.0 Proposal #13483
Closed

MylesBorins added the dont-land-on-v6.x label Jan 11, 2018

kt3k mentioned this pull request Jul 2, 2022
fix(node/fs/exists): fix promisified exists denoland/std#2409
Merged

Uh oh!

Uh oh!

Conversation

dfabulich commented May 30, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfabulichMay 30, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfabulichMay 31, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jun 1, 2017

Uh oh!

vsemozhetbyt commented Jun 1, 2017

Uh oh!

jasnell commented Jun 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

dfabulich commented May 30, 2017•
edited
Loading

dfabulichMay 30, 2017•
edited
Loading

dfabulichMay 31, 2017•
edited
Loading