nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
108k stars 29.8k forks source link

doc: Clarify what <string> means in path option of fs.readFile, fs.readFileSync etc. #36585

Open CxRes opened 3 years ago

CxRes commented 3 years ago

Affected URL(s): https://nodejs.org/dist/latest-v14.x/docs/api/fs.html#fs_fs_readfile_path_options https://nodejs.org/dist/latest-v14.x/docs/api/fs.html#fs_fs_readfilesync_path_options

Description

I am not sure if this a documentation issue or a bug. I get a ENOENT error when I use a url string as path for fs.readFile. However, if I convert the url string to a file path, I am able to read the file.

It needs to be clarified if path strings are exclusively file paths and not urls. If it can also be a url string (and I see no reason for not supporting them?) , then this is bug/feature request!

RaisinTen commented 3 years ago

Did u read this: https://nodejs.org/dist/latest-v14.x/docs/api/fs.html#fs_url_object_support?

CxRes commented 3 years ago

@RaisinTen The link talks about file: url objects. To quote:

For most fs module functions, the path or filename argument may be passed as a WHATWG URL object. Only URL objects using the file: protocol are supported.

I am specifically asking if I provide a file: url string (not url object) e.g. 'file:///path/to/file' instead of '/path/to/file', is that supposed to work? Alternatively, are strings exclusively file paths?

RaisinTen commented 3 years ago

No, that is not supposed to work because string valued paths must exclusively be file paths.

CxRes commented 3 years ago

@RaisinTen But is that documented explicitly? I could not find any elaboration of what the string can be beyond:

path | | | filename or file descriptor

Also, I think a string URL is as valid as file path (contains the same information); Hence, there should be no reason to exclude it.

RaisinTen commented 3 years ago

Isn't filename strictly associated with file paths and not URLs?

CxRes commented 3 years ago

File name is itself not file path. Buffer and URL object are certainly not filenames or file descriptors. Associations and inferences can be argued over till the cows come home. At the very least, I am saying that the documentation can be improved to make these things more explicit. I hope we can agree on that!

gireeshpunathil commented 3 years ago

good discussion so far! I guess the slight difference in perspective comes in by way of how some information are obvious to someone who are familiar with the API versus it being not very explicit and ambiguous to someone else?

IMO, It does not hurt if the <string> in the doc is explicit about its expected format.

RaisinTen commented 3 years ago

Feel free to open a PR to fix this issue if you know how to improve the docs. :)

aduh95 commented 3 years ago

For info, it was chosen that file: URL passed as string would not be parsed as WHATWG URL automatically in case the user has a directory named file: in the current directory:

$ mkdir "file:"
$ echo "relative path" > "./file:/file.txt"
$ echo "absolute path" > /file.txt
$ node -p 'fs.readFileSync("file:///file.txt", "utf8")'
relative path
$ node -p 'fs.readFileSync(new URL("file:///file.txt"), "utf8")'
absolute path
jasnell commented 3 years ago

Specifically from the docs: https://nodejs.org/dist/latest-v15.x/docs/api/fs.html#fs_file_paths

String form paths are interpreted as UTF-8 character sequences
identifying the absolute or relative filename. Relative paths
will be resolved relative to the current working directory as
determined by calling process.cwd().
CxRes commented 3 years ago

@jasnell Thanks for pointing that out. But you would sure appreciate someone looking at say, fs.readFile might miss that and do something like I did up top (use a URL string!)...

May I suggest:

path <string> | <Buffer> | <URL> | <integer> filename or file descriptor

could be better stated as:

path
  <string> absolute or relative path to file (linking back to line you quoted)
  <Buffer> ...
  <URL> ... 
  <integer> file descriptor (linking back to file descriptor)

Also, I may be pedantic but saying" filename" is imho incorrect, filename being the name of the file and not the entire path to it. The text should be:

String form paths are interpreted as UTF-8 character sequences
identifying the absolute or relative *paths to files*. Relative paths
will be resolved relative to the current working directory as
determined by calling process.cwd().
jasnell commented 3 years ago

The docs can always use improvement. That said, the <string> | <Buffer> | <URL> | <integer> structure is generated and cannot really be changed without significant changes throughout the docs. Additional clarifying text can be added tho if you'd like to open a PR

CxRes commented 3 years ago

That's what I feared and is the reason why I am reluctant to open a PR (which would be a lot of work given the change will apply to a number of fs functions). Perhaps the matter can be brought up at TC meeting that individual data types might need exposition and thus should be provisioned for and a format be first agreed upon (No one likes their PR rejected after putting in the labour for a disagreement on conventions)! Additional text "as is" would unavoidably be verbose and might again cause a developer to miss the finer details.

RedYetiDev commented 7 months ago

Hi! Little action has been taken with this issue in a few years, so I'm CCing the relevant teams so they can implement/resolve this issue: @nodejs/documentation @nodejs/fs