doc: add WHATWG file URLs in fs module

[anchnk]

Update fs module documentation adding WHATWG file URLs support for relevant fs functions/classes.

Fixes: #12341
Refs: #10739

Checklist

• documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

doc, fs

Description of change

• Add WHATWG file URLs support in fs root level.

Where relevant :

• Update YAML flags in order to update revision history.
• State that support for this feature is currently experimental.
• Update fs functions / classes arguments types.

[jasnell]

Minor rewording suggestion. Thank you very much for doing this! :-)

[jasnell]

Suggestion:

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. Use of [URL][] objects is currently still experimental. *Note*: `file:` URLs are always absolute paths. 

Then add a link at the end of the page:

[URL]: url.html#url_the_whatwg_url_api 

[anchnk]

@jasnell Updated the PR in order to include your rewording suggestion !

Thanks a lot for helping me out 👍

[TimothyGu]

This is not documentation for a method, but description of the payload of an event. Hence it cannot be a URL.

[anchnk]

Absolutely right, on my way to correct that.

[addaleax]

LGTM with @TimothyGu’s comment addressed

[addaleax]

I think the ; here is a typo, you can remove it while you’re already here if you want

[anchnk]

Good catch @addaleax fixing it as well

[anchnk]

Pushed a new commit including suggestions from @TimothyGu and @addaleax
Also I think the PR has to be run in the CI linter ?
Somebody can take care of that ?
Thank you all for your hints and help and keep going if needed.

[addaleax]

Also I think the PR has to be run in the CI linter ?

It’s a docs-only change, I think we’re good. :) We may have linting for the docs at some point in the future, but we haven’t integrated anything into CI or make test yet.

[benjamingr]

Looks good but would love to see some examples added.

[TimothyGu]

Almost there!

[TimothyGu]

This should maintain the alphabetical order.

[anchnk]

Hi @TimothyGu what alphabetical order ? I was trying to figure it out if there was something like that when I included that change. However i couldn't guess what the logic was as list items seems a bit melted.

Does internal vs external links matters in the order ?
Does functions / classes matters ?

If you can describe me the logical structure beneath the list I will be happy to sort that :)

[sam-github]

the links are un-ordered ATM, sadly, this seems as good a place as any for now

[sam-github]

#12726

[anchnk]

Thank you @sam-github. I will wait that your PR is merged to rebase it in mine.

[TimothyGu]

Aside from the scheme limitation, we also have some other platform-specific restrictions. Documenting them through examples can be helpful.

[anchnk]

I second that. Examples may be helpful. Will work on that tomorrow. Thanks a lot !

[sam-github]

link experimental to the stability index, pls

[anchnk]

@sam-github You mean link experimental to documentation.html#documentation_stability_index ?

[anchnk]

Created this gist with proposal of examples. Feedback is welcome ! Also I plan to add James description in his PR #10739 below the short description i previously made at the root module level.

On Windows, file: URLs with a hostname convert to UNC paths, while file: URLs with drive letters convert to local absolute paths:
file://hostname/a/b/c => \hostname\a\b\c
file:///c:/a/b/c => c:\a\b\c
On all other platforms, file: URLs with a hostname are unsupported and will result in a throw:
file://hostname/a/b/c => throw!
file:///a/b/c => /a/b/c

[sam-github]

Please don't use experimental to describe this feature. That word has a precise definition in node documentation: https://nodejs.org/api/documentation.html#documentation_stability_index and this feature is not gated by a command line flag.

Fwiw, I'm not sure why we need this feature. Unlike https://www.npmjs.com/package/open-uri you need to already have parsed a url into a URL object, checked its scheme and verified its a file.... and at that point, why not just pass the file path from the url directly to the fs call?

[addaleax]

Please don't use experimental to describe this feature.

The WHATWG URL API is experimental, the usage of the term in this PR is correct. In this case, it’s the description of “experimental” in the Stability index that’s wrong – we haven’t been consistent about putting experimental features behind flags (which I am okay with, it can be a case-by-case decision, but we should probably document that better).

Review was for an older iteration of the PR

[jasnell]

The WHATWG URL stuff is still technically experimental, although I'm likely to be opening a PR very soon (ahem, later today) that proposes upgrading it to a fully supported feature.

[sam-github]

I think the URL stuff is technically NOT experimental :-). But as @jasnell says, it will be irrelevant soon.

@anchika That still means you should remove the experimental text.

@addaleax I'm opening an issue seeking confirmation that the text is correct as-is. Until the definition is changed, it is what it is. Full disclosure, I think that we should hide all experimental features behind a flag. Maybe we should talk about at collab summit.

#10739 is absent of any information on why the feature should be added, I still think its odd. Not sure what use-case its for. Does it help browser compatibility? But they don't have fs. Hm, :puzzled-face:.

"experimental" status or not, I doubt we are going to delete this feature now that its here, perhaps the docs can at least say what its useful for.

Also, does the arg actually have to be a URL object, or can it just be an object that has some properties "like" a URL object?

[jasnell]

@sam-github ... this PR should be ready to land before my PR removing the experimental status so the experimental text should actually remain in this PR.

does the arg actually have to be a URL

In practice, yes. See https://github.com/nodejs/node/blob/master/lib/internal/url.js#L1348. The code checks for the presence of the [searchParams] Symbol which is hidden within the internal/url.js file. It would be exceedingly unlikely (tho not impossible) for someone to construct an object that looks like a URL that would work with this.

[anchnk]

Pushed a new commit with platform specific description and examples as requested by @TimothyGu.
For now I will keep the text stating the feature is currently experimental.
Things might change about that but for now it seems ok as it.
I still have to sort the URL link at the end of the file but I can't figure what is the actual sorting order of links. Can somebody help me with that ?

[sam-github]

stability statement are usually linked to stability definitions

[anchnk]

@sam-github Same as above link the word experimental to documentation.html#documentation_stability_index

[jasnell]

@anchnk ... looks like this needs a quick rebase :-)

[anchnk]

@jasnell yup already done locally with amazing support from @addaleax
I will push it once I have sam details so I can enhance the experimental linking :)

[TimothyGu]

1. This PR still doesn't address restrictions like these:

   // On POSIXfs.readFileSync(newURL('file:///path/%2F'));// TypeError [ERR_INVALID_FILE_URL_PATH]: File URL path must not include encoded / characters// On Windowsfs.readFileSync(newURL('file:///C:/path/%2F'));fs.readFileSync(newURL('file:///C:/path/%5C'));// TypeError [ERR_INVALID_FILE_URL_PATH]: File URL path must not include encoded \ or / characters// On Windowsfs.readFileSync(newURL('file:///notdriveletter/path/%5C'));// TypeError [ERR_INVALID_FILE_URL_PATH]: File URL path must be absolute

2. You might want to split this part of the documentation into a new chapter "WHATWG URL object support" (or something like that). The section has grown too big IMO.

[TimothyGu]

nit: extra space before =

[anchnk]

fixed with last commits

[TimothyGu]

camelCase please.

[anchnk]

fixed with last commits

[TimothyGu]

It seems more common to have the drive letter capitalized.

[anchnk]

fixed

[TimothyGu]

Template string literal is unnecessary here. A ' should suffice.

[anchnk]

fixed

[TimothyGu]

Windows

[anchnk]

fixed

[TimothyGu]

IMO it would be easier to use the synchronous API (fs.readFileSync) instead. You can even leave off the encoding argument since it's not necessary to demonstrate URL support.

fs.readFileSync(fileURL);

[anchnk]

In the last commits I tried to remove everything that wasn't meaningful for the examples. So now they are quite minimalistic. That was based on your suggestion, again let me know.

[TimothyGu]

[`URL`][] object

[anchnk]

I created a new chapter on the last commit based on your suggestion @TimothyGu . Also the URL link formatting is fixed.

[addaleax]

You are missing a */ here :)

[anchnk]

Oups ! thank you fixed !

[addaleax]

Can you make sure to not have whitespace before :? ;)

[anchnk]

I did quite a bunch of changes in the last commits, so feel free to give any feedback.
Especially regarding the examples.
Thanks a lot for all your suggestions, help and support.

[TimothyGu]

One last comment. Otherwise LGTM.

[TimothyGu]

This entry shouldn't be necessary anymore.

[anchnk]

Fixed !
Thank you a lot.
Your feedback & suggestions greatly helped me.

[TimothyGu]

Other than the tiniest of nits, LGTM.

[TimothyGu]

Extra space before :

(This is English, not French :)

[anchnk]

Oh so right, forgot about that difference. Sorry :) All fixed in latest commit

[TimothyGu]

Ditto.

[TimothyGu]

Here too.

[TimothyGu]

... and here.

[jasnell]

@anchnk ... can you squash the commits down to prepare for landing?

[anchnk]

@jasnell Squashing done.

[jasnell]

Landed in c1b3b95