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

doc: add WHATWG file URLs in fs module #12670

Closed
wants to merge 1 commit into from

Conversation

@anchnk
Copy link
Contributor

commented Apr 26, 2017

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

Fixes: #12341
Refs: #10739

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

left a comment

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

doc/api/fs.md Outdated
Most fs functions' path or filename arguments can be WHATWG URLs. Only
WHATWG URLs using `file:///` protocol are supported.

*Note* that file WHATWG file URLs are always absolute paths. WHATWG file URLs

This comment has been minimized.

Copy link
@jasnell

jasnell Apr 26, 2017

Member

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

@mscdex mscdex added the whatwg-url label Apr 26, 2017

@anchnk

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2017

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

Thanks a lot for helping me out 👍

doc/api/fs.md Outdated
-->

* `eventType` {string} The type of fs change
* `filename` {string|Buffer} The filename that changed (if relevant/available)
* `filename` {string|Buffer|URL} The filename that changed (if relevant/available)

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Apr 26, 2017

Member

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

This comment has been minimized.

Copy link
@anchnk

anchnk Apr 26, 2017

Author Contributor

Absolutely right, on my way to correct that.

@addaleax
Copy link
Member

left a comment

LGTM with @TimothyGu’s comment addressed

doc/api/fs.md Outdated
@@ -1646,7 +1776,7 @@ changes:
description: The `cache` parameter was removed.
-->

* `path` {string|Buffer};
* `path` {string|Buffer|URL};

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 26, 2017

Member

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

This comment has been minimized.

Copy link
@anchnk

anchnk Apr 26, 2017

Author Contributor

Good catch @addaleax fixing it as well

@anchnk

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2017

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

This comment has been minimized.

Copy link
Member

commented Apr 26, 2017

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

This comment has been minimized.

Copy link
Member

commented Apr 26, 2017

Looks good but would love to see some examples added.

@TimothyGu
Copy link
Member

left a comment

Almost there!

doc/api/fs.md Outdated
@@ -2495,6 +2687,7 @@ The following constants are meant for use with the [`fs.Stats`][] object's
[`stat()`]: fs.html#fs_fs_stat_path_callback
[`util.inspect(stats)`]: util.html#util_util_inspect_object_options
[`WriteStream`]: #fs_class_fs_writestream
[URL]: url.html#url_the_whatwg_url_api

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Apr 26, 2017

Member

This should maintain the alphabetical order.

This comment has been minimized.

Copy link
@anchnk

anchnk Apr 27, 2017

Author Contributor

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 :)

This comment has been minimized.

Copy link
@sam-github

sam-github Apr 28, 2017

Member

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

This comment has been minimized.

Copy link
@sam-github

This comment has been minimized.

Copy link
@anchnk

anchnk Apr 28, 2017

Author Contributor

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

doc/api/fs.md Outdated
@@ -70,6 +70,12 @@ the entire process until they complete--halting all connections.
The relative path to a filename can be used. Remember, however, that this path
will be relative to `process.cwd()`.

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.

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Apr 26, 2017

Member

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

This comment has been minimized.

Copy link
@anchnk

anchnk Apr 26, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@sam-github

sam-github Apr 28, 2017

Member

link experimental to the stability index, pls

This comment has been minimized.

Copy link
@anchnk

anchnk Apr 28, 2017

Author Contributor

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

@TimothyGu TimothyGu added this to doc / benchmark / test in WHATWG URL implementation Apr 27, 2017

@anchnk

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2017

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

This comment has been minimized.

Copy link
Member

commented Apr 27, 2017

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

This comment has been minimized.

Copy link
Member

commented Apr 27, 2017

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

This comment has been minimized.

Copy link
Member

commented Apr 27, 2017

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

This comment has been minimized.

Copy link
Member

commented Apr 27, 2017

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

This comment has been minimized.

Copy link
Member

commented Apr 27, 2017

@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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2017

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 ?

- version: v7.6.0
pr-url: https://github.com/nodejs/node/pull/10739
description: The `path` parameter can be a WHATWG `URL` object using `file:`
protocol. Support is currently still *experimental*.

This comment has been minimized.

Copy link
@sam-github

sam-github Apr 28, 2017

Member

stability statement are usually linked to stability definitions

This comment has been minimized.

Copy link
@anchnk

anchnk Apr 28, 2017

Author Contributor

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

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 28, 2017

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

@anchnk

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2017

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

left a comment

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

    // On POSIX
    fs.readFileSync(new URL('file:///path/%2F'));
    // TypeError [ERR_INVALID_FILE_URL_PATH]: File URL path must not include encoded / characters
    
    // On Windows
    fs.readFileSync(new URL('file:///C:/path/%2F'));
    fs.readFileSync(new URL('file:///C:/path/%5C'));
    // TypeError [ERR_INVALID_FILE_URL_PATH]: File URL path must not include encoded \ or / characters
    
    // On Windows
    fs.readFileSync(new URL('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.

doc/api/fs.md Outdated
supported. Use of [URL][] objects is currently still experimental.

```js
const fs = require('fs');

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Apr 29, 2017

Member

nit: extra space before =

This comment has been minimized.

Copy link
@anchnk

anchnk Apr 30, 2017

Author Contributor

fixed with last commits

doc/api/fs.md Outdated
```js
const fs = require('fs');
const { URL } = require('url');
const file_url = new URL('file:///tmp/hello');

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Apr 29, 2017

Member

camelCase please.

This comment has been minimized.

Copy link
@anchnk

anchnk Apr 30, 2017

Author Contributor

fixed with last commits

doc/api/fs.md Outdated
const file_url_with_hostname = new URL('file://hostname/p/a/t/h/file');

// path with drive letter converted to absolute path => c:\tmp\hello
const file_url_with_drive_letter = new URL('file:///c:/tmp/hello');

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Apr 29, 2017

Member

It seems more common to have the drive letter capitalized.

This comment has been minimized.

Copy link
@anchnk

anchnk Apr 30, 2017

Author Contributor

fixed

doc/api/fs.md Outdated
const { URL } = require('url');

// path with hostname => throw!
const file_url_with_hostname = new URL(`file://hostname/p/a/t/h/file`);

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Apr 29, 2017

Member

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

This comment has been minimized.

Copy link
@anchnk

anchnk Apr 30, 2017

Author Contributor

fixed

doc/api/fs.md Outdated
// path with drive letter converted to absolute path => c:\tmp\hello
const file_url_with_drive_letter = new URL('file:///c:/tmp/hello');

// On windows, WHATWG file URLs with hostname convert to UNC path

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Apr 29, 2017

Member

Windows

This comment has been minimized.

Copy link
@anchnk

anchnk Apr 30, 2017

Author Contributor

fixed

doc/api/fs.md Outdated
const { URL } = require('url');
const file_url = new URL('file:///tmp/hello');

fs.readFile(file_url, 'utf-8', (err,data) => {

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Apr 29, 2017

Member

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);

This comment has been minimized.

Copy link
@anchnk

anchnk Apr 30, 2017

Author Contributor

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.

doc/api/fs.md Outdated
@@ -70,6 +70,76 @@ the entire process until they complete--halting all connections.
The relative path to a filename can be used. Remember, however, that this path
will be relative to `process.cwd()`.

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

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Apr 29, 2017

Member
[`URL`][] object

This comment has been minimized.

Copy link
@anchnk

anchnk Apr 30, 2017

Author Contributor

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

@anchnk anchnk force-pushed the anchnk:master branch Apr 29, 2017

doc/api/fs.md Outdated
fs.readFileSync(new URL('file:///p/a/t/h/%2F'));
fs.readFileSync(new URL('file:///p/a/t/h/%2f'));
/* TypeError [ERR_INVALID_FILE_URL_PATH]: File URL path must not include encoded
/ characters *:

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 29, 2017

Member

You are missing a */ here :)

This comment has been minimized.

Copy link
@anchnk

anchnk Apr 30, 2017

Author Contributor

Oups ! thank you fixed !

doc/api/fs.md Outdated
/* TypeError [ERR_INVALID_FILE_URL_PATH]: File URL path must not include encoded
/ characters *:
```
On Windows, `file:` URLs having encoded backslash will result in a throw :

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 29, 2017

Member

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

@anchnk

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2017

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

left a comment

One last comment. Otherwise LGTM.

doc/api/fs.md Outdated
@@ -2499,6 +2774,7 @@ The following constants are meant for use with the [`fs.Stats`][] object's
[`stat()`]: fs.html#fs_fs_stat_path_callback
[`util.inspect(stats)`]: util.html#util_util_inspect_object_options
[`WriteStream`]: #fs_class_fs_writestream
[URL]: url.html#url_the_whatwg_url_api

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Apr 30, 2017

Member

This entry shouldn't be necessary anymore.

This comment has been minimized.

Copy link
@anchnk

anchnk Apr 30, 2017

Author Contributor

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

@anchnk anchnk force-pushed the anchnk:master branch Apr 30, 2017

@TimothyGu
Copy link
Member

left a comment

Other than the tiniest of nits, LGTM.

doc/api/fs.md Outdated
the drive letter. Using another separator will result in a throw.

On all other platforms, `file:` URLs with a hostname are unsupported and will
result in a throw :

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Apr 30, 2017

Member

Extra space before :

(This is English, not French :)

This comment has been minimized.

Copy link
@anchnk

anchnk May 1, 2017

Author Contributor

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

doc/api/fs.md Outdated
result in a throw :

```js
// On other platforms :

This comment has been minimized.

Copy link
@TimothyGu
doc/api/fs.md Outdated
```

A `file:` URL having encoded slash characters will result in a throw on all
platforms :

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Apr 30, 2017

Member

Here too.

doc/api/fs.md Outdated
/* TypeError [ERR_INVALID_FILE_URL_PATH]: File URL path must not include encoded
/ characters */
```
On Windows, `file:` URLs having encoded backslash will result in a throw :

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Apr 30, 2017

Member

... and here.

@anchnk anchnk force-pushed the anchnk:master branch May 1, 2017

@jasnell

This comment has been minimized.

Copy link
Member

commented May 4, 2017

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

Olivier Martin
doc: add WHATWG file URLs in fs module
Update fs module documentation adding WHATWG file URLS support for
relevant fs functions/classes.

Fixes: #12341
Refs: #10739

@anchnk anchnk force-pushed the anchnk:master branch to b2257f9 May 4, 2017

@anchnk

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2017

@jasnell Squashing done.

jasnell added a commit that referenced this pull request May 4, 2017
doc: add WHATWG file URLs in fs module
Update fs module documentation adding WHATWG file URLS support for
relevant fs functions/classes.

PR-URL: #12670
Fixes: #12341
Ref: #10739
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@jasnell

This comment has been minimized.

Copy link
Member

commented May 4, 2017

Landed in c1b3b95

@jasnell jasnell closed this May 4, 2017

@TimothyGu TimothyGu referenced this pull request May 8, 2017

@watilde watilde moved this from doc / benchmark / test to Done in WHATWG URL implementation May 8, 2017

@jasnell jasnell referenced this pull request May 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.