fs: undeprecate existsSync #8364

dfabulich · 2016-08-31T23:11:46Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

fs

Description of change

Undeprecate fs.existsSync, apropos @Fishrock123's comment on PR #7455

dfabulich · 2016-08-31T23:21:42Z

For those not following along with the multiple giant threads around this, I think my summary comment tells the clearest story:

There's an enormous gripe thread about the deprecation of existsSync in issue gripe: deprecating fs.exists/existsSync #1592, which is summarized pretty well in PR fs: add accessibleSync() which returns a boolean #4217.

accessSync throws an exception if the file isn't accessible — you're asserting that the file exists and is accessible. But most developers don't want/need the actual exception object, so they just discard it. (Look at the existing implementation of existsSync.)

Generating an exception just so you can ignore it hurts performance. In PR #4217 we're considering adding a new method accessibleSync that would return a boolean (fast!) instead, but that would add unnecessary methods to Node's core, when we could just undeprecate existsSync.

Furthermore, accessSync is just plain annoying to use for ordinary use cases.
if (fs.existsSync(".git/rebase-apply/rebasing")) {
  console.error("You are in the middle of a rebase!");
}
To write that with accessSync you have to use a catch block.
try {
  fs.accessSync(".git/rebase-apply/rebasing");
} catch (ignored) {
  console.error("You are in the middle of a rebase!");
}
Perhaps if annoyance were the only problem, we could all just use a one-liner npm module for existsSync, but there's also performance to consider. You just can't make accessSync fast without making it return a boolean, like existsSync does.

Today, exists uses stat under the hood, returning false if stat returns an error. It would be faster to make exists use access under the hood, which is PR #7455, but this may be risky; it's rumored that access may be able to succeed where stat fails, particularly on Windows where it might be possible to configure permissions in such a way that access succeeds but stat fails.

Hence this PR, to simply undeprecate existsSync.

Fishrock123 · 2016-08-31T23:49:00Z

@nodejs/ctc I find the arguments here compelling and would like to follow through.

yosuke-furukawa · 2016-09-01T00:41:17Z

IMHO, our fs is a wrapper of posix standard API.

exists is easy API, that is really helpful for development. But POSIX API does not have exists.

And, we would be better to keep core simple. we should not re-support those easy API like exists.

dfabulich · 2016-09-01T05:32:50Z

It may surprise you that I mostly agree with this. In PR #4217, I think we reached some consensus that it would have been better if accessibleSync had been part of the API all along.

But the problem now is that node is doing too much on top of the POSIX API. access wraps access(2) in an asynchronous API; accessSync calls access(2) but then throws an exception on failure; access(2) doesn't do that because C doesn't even have exceptions.

What we need is an API that calls access(2) and then returns a value, like POSIX does.

That either means adding a new accessibleSync function to core, or undeprecating existsSync.

Undeprecating existsSync is the least bad thing we can do; it's even better than nothing.

jasnell · 2016-09-01T15:11:00Z

LGTM

thefourtheye · 2016-09-01T16:07:49Z

Deprecating only the async version would be confusing to the new users I believe. The doc just points to the exists documentation, which is actually deprecated.

Fishrock123 · 2016-09-01T16:11:08Z

We can add the relevant documentation to this method... (We're not gona undeprecate fs.exists()...)

dfabulich · 2016-09-01T16:35:53Z

Added relevant documentation to fs.exists() and fs.existsSync(), clarifying that the callback of fs.exists() is non-standard, but fs.existsSync() does not use a callback.

thefourtheye · 2016-09-01T16:50:29Z

doc/api/fs.md

@@ -693,6 +693,12 @@ fs.exists('/etc/passwd', (exists) => {
 });
 ```

+**Note that the parameter to this callback is not consistent with other Node


Thanks, fixed.

jasnell · 2016-09-01T19:49:41Z

doc/api/fs.md

 * `path` {String | Buffer}

 Synchronous version of [`fs.exists()`][].
 Returns `true` if the file exists, `false` otherwise.

+Note that `fs.exists()` is deprecated, but `fs.existsSync()` is not.
+(The `callback` parameter to `fs.exists()` accepts parameters that are
+inconsistent with other Node callbacks. `fs.existsSync()` does not use


Another one ;-) s/Node/Node.js/g

Trott · 2016-09-02T00:21:25Z

Are doc-only deprecations semver-major? If so, then should undoing a doc-only deprecation like this also be semver-major? I don't have an opinion either way; just seeking clarity.

ChALkeR · 2016-09-05T07:56:41Z

If so, then should undoing a doc-only deprecation like this also be semver-major?

No, undoing a semver-major generally should be either semver-minor (in most cases) or semver-patch (in case if the breakage that is being reverted was unintended). Likewise, undoing a semver-minor is generally a semver-major.

Undoing a semver-major is a semver-major only in cases of an API behaviour change which is not an addition, deprecation or removal of something — and something like that should generally be avoided, if possible.

But in this specific situation (where the deprecation was docs-only) we could just land that to all branches starting from 4.x in a patch release, IMO.

dfabulich · 2016-09-05T20:02:11Z

Fixed the last outstanding s/Node/Node.js/

dfabulich · 2016-09-19T05:18:35Z

ping?

Trott · 2016-09-19T06:20:04Z

Are any @nodejs/collaborators strongly in favor of undeprecating fs.existsSync() as proposed here? There are differences of opinion among collaborators on this. Without a collaborator champion to help work things through the process, this is unlikely to land.

I see @jasnell gave this a LGTM but I'm not sure that he's willing to champion this. (Hey, @jasnell, are you?)

Anyone else?

dfabulich · 2016-09-19T06:28:48Z

I thought (hoped?) @Fishrock123 was the relevant champion here.

Trott · 2016-09-19T06:31:38Z

I thought (hoped?) @Fishrock123 was the relevant champion here.

Good point. @Fishrock123 assigned the issue to themselves as well, which seems like a pretty good indication.

It wouldn't hurt to have more champions, I suppose. :-D

jasnell · 2016-09-19T07:06:46Z

I believe this should be done. If that's the same as championing, then ok 😃

On Monday, September 19, 2016, Rich Trott notifications@github.com wrote:

I thought (hoped?) @Fishrock123 https://github.com/Fishrock123 was the
relevant champion here.

Good point. @Fishrock123 https://github.com/Fishrock123 assigned the
issue to themselves as well, which seems like a pretty good indication.

It wouldn't hurt to have more champions, I suppose. :-D

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#8364 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2eUp_SYyeDwmCbhY0FH-kNTiZC89Eks5qrixRgaJpZM4JyKL5
.

imyller · 2016-09-19T07:16:29Z

I'm quite new-in-town, but I'm also in favor of undeprecating fs.existsSync.

LGTM

mcollina · 2016-09-19T08:01:38Z

LGTM

Trott · 2016-09-19T23:13:00Z

OK, that's three LGTMs and some additional supportive comments from others.

Do any Collaborators object? That is, are any Collaborators opposed to it to the point that they would veto it? Something stronger than just "I don't like it but I'm not going to stop it if others want it".

/cc @seishun @yosuke-furukawa @thefourtheye

dfabulich · 2016-10-03T21:22:52Z

doc/api/fs.md: no issues found

Fishrock123 · 2016-10-04T14:06:47Z

@ChALkeR can you approve so I can merge? Thanks :)

ChALkeR · 2016-10-04T15:12:57Z

@Fishrock123, @dfabulich, @Trott, sorry for the delay. LGTM now.

The PR with the doclint tooling is in #8551, if you will want that again until it's merged =).
Removing the trailing / for manual checking is fine, it's needed for the tooling to function correctly (#8666 explains why).

@Fishrock123

This has been dragged through various long discussions and has been elevated to the CTC multiple times. As noted in #7455 (comment), while this API is still generally considered an anti-pattern, there are still use-cases it is best suited for, such as checking if a git rebase is in progress by looking if ".git/rebase-apply/rebasing" exists. The general consensus is to undeprecate just the sync version, given that the async version still has the "arguments order inconsistency" problem. The consensus at the two last CTC meetings this came up at was also to undeprecate existsSync() but keep exists() deprecated. See: #8242 & #8330 (Description write-up by @Fishrock123) Fixes: #1592 Refs: #4217 Refs: #7455 PR-URL: #8364 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

Fishrock123 · 2016-10-05T19:12:15Z

Landed in 7b5ffa4. Hopefully this will put it to rest (for the most part).

(I wrote an extensive summary into the commit description.)

Thanks so much @dfabulich for helping push this forward. :)

@Fishrock123

This has been dragged through various long discussions and has been elevated to the CTC multiple times. As noted in #7455 (comment), while this API is still generally considered an anti-pattern, there are still use-cases it is best suited for, such as checking if a git rebase is in progress by looking if ".git/rebase-apply/rebasing" exists. The general consensus is to undeprecate just the sync version, given that the async version still has the "arguments order inconsistency" problem. The consensus at the two last CTC meetings this came up at was also to undeprecate existsSync() but keep exists() deprecated. See: #8242 & #8330 (Description write-up by @Fishrock123) Fixes: #1592 Refs: #4217 Refs: #7455 PR-URL: #8364 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

@Fishrock123

This has been dragged through various long discussions and has been elevated to the CTC multiple times. As noted in #7455 (comment), while this API is still generally considered an anti-pattern, there are still use-cases it is best suited for, such as checking if a git rebase is in progress by looking if ".git/rebase-apply/rebasing" exists. The general consensus is to undeprecate just the sync version, given that the async version still has the "arguments order inconsistency" problem. The consensus at the two last CTC meetings this came up at was also to undeprecate existsSync() but keep exists() deprecated. See: #8242 & #8330 (Description write-up by @Fishrock123) Fixes: #1592 Refs: #4217 Refs: #7455 PR-URL: #8364 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

* fs: - `SyncWriteStream` now inherits from `Stream.Writable`. (Anna Henningsen) #8830 - Practically, this means that when stdio is piped to a file, stdout and stderr will still be `Writable` streams. - `fs.existsSync()` has been undeprecated. `fs.exists()` remains deprecated. (Dan Fabulich) #8364 * http: `http.request()` now accepts a `timeout` option. (Rene Weber) #8101 * module: The module loader now maintains its own realpath cache. (Anna Henningsen) #8100 * npm: Upgraded to 3.10.8 (Kat Marchán) #8706 * stream: `Duplex` streams now show proper `instanceof Stream.Writable`. (Anna Henningsen) #8834 * timers: Improved `setTimeout`/`Interval` performance by up to 22%. (Brian White) #8661 PR-URL: #9034

* fs: - `SyncWriteStream` now inherits from `Stream.Writable`. (Anna Henningsen) nodejs/node#8830 - Practically, this means that when stdio is piped to a file, stdout and stderr will still be `Writable` streams. - `fs.existsSync()` has been undeprecated. `fs.exists()` remains deprecated. (Dan Fabulich) nodejs/node#8364 * http: `http.request()` now accepts a `timeout` option. (Rene Weber) nodejs/node#8101 * module: The module loader now maintains its own realpath cache. (Anna Henningsen) nodejs/node#8100 * npm: Upgraded to 3.10.8 (Kat Marchan) nodejs/node#8706 * stream: `Duplex` streams now show proper `instanceof Stream.Writable`. (Anna Henningsen) nodejs/node#8834 * timers: Improved `setTimeout`/`Interval` performance by up to 22%. (Brian White) nodejs/node#8661 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

It's not deprecated anymore, see nodejs/node#8364

See nodejs/node#8364

It's not deprecated anymore, see nodejs/node#8364

nodejs-github-bot added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Aug 31, 2016

dfabulich mentioned this pull request Aug 31, 2016

fs: undeprecate existsSync; use access instead of stat for performance #7455

Closed

4 tasks

dfabulich force-pushed the undeprecate-exists-sync branch from dbe9f8c to d75e0a3 Compare September 1, 2016 16:34

thefourtheye reviewed Sep 1, 2016
View reviewed changes

dfabulich force-pushed the undeprecate-exists-sync branch 2 times, most recently from 2cd5cc1 to db6d9e6 Compare September 1, 2016 18:41

jasnell reviewed Sep 1, 2016
View reviewed changes

dfabulich force-pushed the undeprecate-exists-sync branch from db6d9e6 to 0bb3e9d Compare September 5, 2016 20:01

Fishrock123 self-assigned this Sep 7, 2016

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 9, 2016

ChALkeR approved these changes Oct 4, 2016

View reviewed changes

Fishrock123 mentioned this pull request Oct 5, 2016

fs: add accessibleSync() which returns a boolean #4217

Closed

Fishrock123 closed this Oct 5, 2016

SimenB mentioned this pull request Oct 15, 2016

Use the undeprecated fs.accessSync sindresorhus/path-exists#7

Merged

SimenB added a commit to SimenB/babel that referenced this pull request Oct 15, 2016

Replace path-exists with fs.existsSync

69e41d3

It's not deprecated anymore, see nodejs/node#8364

SimenB mentioned this pull request Oct 15, 2016

Replace path-exists with fs.existsSync babel/babel#4731

Merged

astorije mentioned this pull request Oct 16, 2016

fs.existsSync() is deprecated thelounge/thelounge#672

Closed

lgeiger mentioned this pull request Oct 16, 2016

Better cli interface nteract/nteract#1028

Merged

SimenB added a commit to SimenB/babel that referenced this pull request Oct 17, 2016

Replace path-exists with fs.existsSync

59f2526

It's not deprecated anymore, see nodejs/node#8364

sindresorhus pushed a commit to sindresorhus/path-exists that referenced this pull request Oct 17, 2016

Recommend fs.existsSync (#7)

67c0725

See nodejs/node#8364

sindresorhus pushed a commit to sindresorhus/path-exists that referenced this pull request Oct 17, 2016

Recommend fs.existsSync (#7)

ed99497

See nodejs/node#8364

hzoo pushed a commit to babel/babel that referenced this pull request Oct 17, 2016

Replace path-exists with fs.existsSync (#4731)

9c2794d

It's not deprecated anymore, see nodejs/node#8364

chrisprice pushed a commit to chrisprice/babel that referenced this pull request Oct 18, 2016

Replace path-exists with fs.existsSync (babel#4731)

e4eca7c

It's not deprecated anymore, see nodejs/node#8364

This was referenced Nov 4, 2016

Use fs.statSync? ember-cli/exists-sync#2

Closed

Update exists-sync. Add relative path test. broccolijs/broccoli-funnel#82

Merged

lgeiger mentioned this pull request Dec 6, 2016

Cleanup prebuild script prebuild/prebuild#142

Merged

panagosg7 pushed a commit to panagosg7/babel that referenced this pull request Jan 17, 2017

Replace path-exists with fs.existsSync (babel#4731)

becaf97

It's not deprecated anymore, see nodejs/node#8364

twokul mentioned this pull request Nov 12, 2017

Update README.md ember-cli/exists-sync#9

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: undeprecate existsSync #8364

fs: undeprecate existsSync #8364

dfabulich commented Aug 31, 2016

dfabulich commented Aug 31, 2016

Fishrock123 commented Aug 31, 2016 •

edited

Loading

yosuke-furukawa commented Sep 1, 2016

dfabulich commented Sep 1, 2016

jasnell commented Sep 1, 2016

thefourtheye commented Sep 1, 2016

Fishrock123 commented Sep 1, 2016 •

edited

Loading

dfabulich commented Sep 1, 2016

thefourtheye Sep 1, 2016

dfabulich Sep 1, 2016

jasnell Sep 1, 2016

Trott commented Sep 2, 2016

ChALkeR commented Sep 5, 2016 •

edited

Loading

dfabulich commented Sep 5, 2016

dfabulich commented Sep 19, 2016

Trott commented Sep 19, 2016

dfabulich commented Sep 19, 2016

Trott commented Sep 19, 2016

jasnell commented Sep 19, 2016

imyller commented Sep 19, 2016

mcollina commented Sep 19, 2016

Trott commented Sep 19, 2016

dfabulich commented Oct 3, 2016

Fishrock123 commented Oct 4, 2016

ChALkeR commented Oct 4, 2016 •

edited

Loading

Fishrock123 commented Oct 5, 2016 •

edited

Loading

fs: undeprecate existsSync #8364

fs: undeprecate existsSync #8364

Conversation

dfabulich commented Aug 31, 2016

Checklist

Affected core subsystem(s)

Description of change

dfabulich commented Aug 31, 2016

Fishrock123 commented Aug 31, 2016 • edited Loading

yosuke-furukawa commented Sep 1, 2016

dfabulich commented Sep 1, 2016

jasnell commented Sep 1, 2016

thefourtheye commented Sep 1, 2016

Fishrock123 commented Sep 1, 2016 • edited Loading

dfabulich commented Sep 1, 2016

thefourtheye Sep 1, 2016

Choose a reason for hiding this comment

dfabulich Sep 1, 2016

Choose a reason for hiding this comment

jasnell Sep 1, 2016

Choose a reason for hiding this comment

Trott commented Sep 2, 2016

ChALkeR commented Sep 5, 2016 • edited Loading

dfabulich commented Sep 5, 2016

dfabulich commented Sep 19, 2016

Trott commented Sep 19, 2016

dfabulich commented Sep 19, 2016

Trott commented Sep 19, 2016

jasnell commented Sep 19, 2016

imyller commented Sep 19, 2016

mcollina commented Sep 19, 2016

Trott commented Sep 19, 2016

dfabulich commented Oct 3, 2016

Fishrock123 commented Oct 4, 2016

ChALkeR commented Oct 4, 2016 • edited Loading

Fishrock123 commented Oct 5, 2016 • edited Loading

Fishrock123 commented Aug 31, 2016 •

edited

Loading

Fishrock123 commented Sep 1, 2016 •

edited

Loading

ChALkeR commented Sep 5, 2016 •

edited

Loading

ChALkeR commented Oct 4, 2016 •

edited

Loading

Fishrock123 commented Oct 5, 2016 •

edited

Loading