#goodnessSquad - added warning to fs.accessSync with link for instructions about blo…

[JonathanDn]

…cking calls

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)

[cjihrig]

I don't think I understand why this is being added, and why it is only being added to one of the sync functions.

[JonathanDn]

Hey @cjihrig , this is a fix for issue #1684 , I am now adding this warning to all other sync functions in the api docs.

[Trott]

Hi, @JonathanDn! Welcome and thanks for this! The text seems like it can stand some improvements. Here are my suggestions.

• Please remove the comma splice.
• I'd also remove the typographic emphases (bold text, starting with a "Warning: " label), as we overuse those in our documents to an almost comical degree.
• Avoid link text of "here". Indicate what it is.

All told, something like this?:

Note that this is a blocking call. Avoid this method in an asynchronous
environment. See
[Overview of Blocking vs Non-Blocking](https://nodejs.org/en/docs/guides/blocking-vs-non-blocking)
for more information.

[JonathanDn]

Thanks a whole lot for the feedback Trott ! 👍

[vsemozhetbyt]

blocking). <- this period seems erroneous here and in all the other places.

[vsemozhetbyt]

And maybe it is worth to use a bottom reference instead of repeating the link URL: this will reduce the file size and the length of the lines.

[JonathanDn]

Thanks I am fixing it now. 👍

[vsemozhetbyt]

These lines are 81 characters now, so I am not sure the doc linting would be happy with it. Maybe the link can be just [Overview of Blocking vs Non-Blocking][] and the bottom reference [Overview of Blocking vs Non-Blocking]: https://...?

[JonathanDn]

I did not know it could work this way, sure that makes sense Ill turn it to:
[Overview of Blocking vs Non-Blocking][] on all the references.

[vsemozhetbyt]

Here are some details: https://daringfireball.net/projects/markdown/syntax#link

[vsemozhetbyt]

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

[JonathanDn]

Hey @vsemozhetbyt excuse my ignorance, but is this test build a procedure this is required before merging a PR?

[vsemozhetbyt]

@JonathanDn Yes. CI contains doc linting and a test for building docs required for PR adoption. You can read more about all the process here.

[JonathanDn]

Cool @vsemozhetbyt thanks for the elaboration.

[JonathanDn]

@vsemozhetbyt so some checks were unsuccessful, anything I need to fix or what's the drill for my PR to be accepted?

[vsemozhetbyt]

@JonathanDn CI failures seem doc-unrelated. Now we need to wait till some collaborators approve the PR. Doc format LGTM, but let us see if others think we need these notes for all the sync methods in fs module only (as we have sync methods in other modules as well).

[bnoordhuis]

I appreciate the work but it seems mostly redundant to me. The synchronicity is made explicit in the Sync suffix and the synopsis already explains why you should not normally use them.

I also have reservations about "asynchronous environment" - it's too unspecific to make a good warning.

[BridgeAR]

I am really not a fan of this and would rather not want to land this. It is not only redundant but also not really helpful out of my perspective. And as @bnoordhuis pointed out a asynchronous environment is really not a good description. Node.js itself is a asynchronous environment and that would imply that those methods should never be used at all.

I would strongly prefer a general description that points out the difference between sync and async at the top of the documentation.

[bnoordhuis]

@jasnell Several collaborators spoke out against this change. It would be useful to know why you are in favor.

[fhinkel]

I second @bnoordhuis sentiment and would prefer not to land this.

[JonathanDn]

Hey there @bnoordhuis, what kind of wording change would you prefer over "asynchronous environment?

In my opinion a literal suffix for a function just to state weather it is async/ sync is redundant, further it also kind of makes the function name un-pure, if all a function does is "read a directory / file" then it should be called "readdir()" or "readFile()", adding a suffix like so: "readFileAsync()" / readdirAsync()" or with the Sync suffix implies of further redundant information about the way the function works rather then it's simple pure action - 1. reading from a directory 2. reading from a file.

I would prefer a well documented API Docs to understand the usage of a function I am thinking to use, and a clean and pure function name with it.

[bnoordhuis]

@JonathanDn It's not entirely clear to me what point you are trying to get across. The function names are what they are and won't change.

readFileAsync() is redundant because everything in node.js is async by default. readFileSync() is not redundant because it's the outlier - and also, the Sync suffix stands out and hopefully is enough of an eye sore to be a deterrent.

[benjamingr]

@bnoordhuis I tend to agree that reading it everywhere can be annoying but I also see the value in it - what about adding a warning icon ⚠️ and when hovered over it would show the warning?

[bnoordhuis]

Hover-overs are arguably not so nice to the visually impaired.

Let's take a step back: do we feel the synopsis is lacking? If yes, that should be first thing to improve. If the synopsis is okay, then why is it not sufficient?

[BridgeAR]

Right now there were multiple collaborators who spoke out against this. I do not feel like this will land as is without a significant rework.

@jasnell would you please take another look? #17503 (comment)

[BridgeAR]

@JonathanDn right now I do not see this landing as there are multiple -1. So I am sorry but I am going to close this as is. Thanks a lot for your contribution anyway! It is much appreciated.

If you would like to work on this again to add a generic part, please either comment here or open a new PR (the latter is probably preferable). Such a change would also have a much higher chance of getting merged!