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: undeprecate existsSync #8364

Closed
wants to merge 1 commit into from

Conversation

dfabulich
Copy link
Contributor

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

@nodejs-github-bot 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
Copy link
Contributor Author

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

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

  2. 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
Copy link
Contributor

Fishrock123 commented Aug 31, 2016

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

@yosuke-furukawa
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

jasnell commented Sep 1, 2016

LGTM

@thefourtheye
Copy link
Contributor

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
Copy link
Contributor

Fishrock123 commented Sep 1, 2016

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

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

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.

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

**Note that the parameter to this callback is not consistent with other Node
Copy link
Contributor

Choose a reason for hiding this comment

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

Node.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed.

@dfabulich dfabulich force-pushed the undeprecate-exists-sync branch 2 times, most recently from 2cd5cc1 to db6d9e6 Compare September 1, 2016 18:41
* `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
Copy link
Member

Choose a reason for hiding this comment

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

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

@Trott
Copy link
Member

Trott commented Sep 2, 2016

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
Copy link
Member

ChALkeR commented Sep 5, 2016

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 dfabulich force-pushed the undeprecate-exists-sync branch from db6d9e6 to 0bb3e9d Compare September 5, 2016 20:01
@dfabulich
Copy link
Contributor Author

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

@Fishrock123 Fishrock123 self-assigned this Sep 7, 2016
@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 9, 2016
@dfabulich
Copy link
Contributor Author

ping?

@Trott
Copy link
Member

Trott commented Sep 19, 2016

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
Copy link
Contributor Author

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

@Trott
Copy link
Member

Trott commented Sep 19, 2016

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
Copy link
Member

jasnell commented Sep 19, 2016

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
Copy link
Member

imyller commented Sep 19, 2016

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

LGTM

@mcollina
Copy link
Member

LGTM

@Trott
Copy link
Member

Trott commented Sep 19, 2016

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
Copy link
Contributor Author

doc/api/fs.md: no issues found

@Fishrock123
Copy link
Contributor

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

@ChALkeR
Copy link
Member

ChALkeR commented Oct 4, 2016

@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 pushed a commit that referenced this pull request Oct 5, 2016
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
Copy link
Contributor

Fishrock123 commented Oct 5, 2016

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 Fishrock123 closed this Oct 5, 2016
jasnell pushed a commit that referenced this pull request Oct 6, 2016
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 pushed a commit that referenced this pull request Oct 11, 2016
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 added a commit that referenced this pull request Oct 12, 2016
* 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
Fishrock123 added a commit that referenced this pull request Oct 12, 2016
* 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
imyller added a commit to imyller/meta-nodejs that referenced this pull request Oct 13, 2016
    * 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>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Oct 13, 2016
    * 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>
SimenB added a commit to SimenB/babel that referenced this pull request Oct 15, 2016
SimenB added a commit to SimenB/babel that referenced this pull request Oct 17, 2016
sindresorhus pushed a commit to sindresorhus/path-exists that referenced this pull request Oct 17, 2016
sindresorhus pushed a commit to sindresorhus/path-exists that referenced this pull request Oct 17, 2016
hzoo pushed a commit to babel/babel that referenced this pull request Oct 17, 2016
chrisprice pushed a commit to chrisprice/babel that referenced this pull request Oct 18, 2016
panagosg7 pushed a commit to panagosg7/babel that referenced this pull request Jan 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.