fs: allow WHATWG URL and file: URLs as paths by jasnell · Pull Request #10739 · nodejs/node · GitHub

Updates the fs module APIs to allow 'file://' URL strings and WHATWG URL objects using 'file:' protocol to be passed as the path.

For example:

const URL = require('url').URL;
const myURL = new URL('file:///C:/path/to/file');
fs.readFile(myURL, (err, data) => {});

// or
// EDIT(addaleax): this was removed from this PR
fs.readFile('file:///C:/path/to/file', (err, data) => {});

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

The documentation for the fs API is intentionally not updated in this commit because the URL API is still considered experimental and is not officially documented at this time

Note that file: URLs are required by spec to always be absolute paths from the file system root.

This is a semver-major commit because it changes error handling on the fs APIs.

Refs: #10703

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)

fs, url

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

I think the url label can be removed here? (Also, can you take a look at nodejs/github-bot#115 so that the bot will stop labeling these PRs with url?)

heh, yeah, the bot seems to be a bit aggressive about labeling things these days

New CI: https://ci.nodejs.org/job/node-test-pull-request/5811/
(previous one had an error on windows)

The WHATWG URL parsing could escape the null byte if it's not in the hostname ('\u0000' -> '%00', e.g. new URL('file://hostname/a/b/c\u0000')), thus bypassing the null check, so might worth add a test for it. Is it possible for getPathFromURL to put null bytes in a path without one? If not, maybe the nullCheck can be performed before getPathFromURL.

good point. it's not just about null bytes, the path could contain any number of pct-encoded characters. This should run the path through a decode before returning... will fix that

Yes, that can be a problem for files with non-ASCII names...might worth add a test for something like 'file://hostname/文件.txt'.

Also, the hostname can be escaped by domain to ASCII too. Like 'file://你好你好/a/b'

Why not path.codePointAt(0)? Or path.startsWith('file://')?

Maybe this can be moved into the else if (path instanceof URL) block, because a path that starts with file://(isFileUrl ) will have the file protocol.

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

sigh... lol... obvious that last run was less than successful ;-) ... will investigate later on tonight

Notable changes: * deps: * update V8 to 5.5 (Michaël Zasso) [nodejs#11029](nodejs#11029) * upgrade libuv to 1.11.0 (cjihrig) [nodejs#11094](nodejs#11094) * add node-inspect 1.10.4 (Jan Krems) [nodejs#10187](nodejs#10187) * upgrade zlib to 1.2.11 (Sam Roberts) [nodejs#10980](nodejs#10980) * lib: build `node inspect` into `node` (Anna Henningsen) [nodejs#10187](nodejs#10187) * crypto: Remove expired certs from CNNIC whitelist (Shigeki Ohtsu) [nodejs#9469](nodejs#9469) * inspector: add --inspect-brk (Josh Gavant) [nodejs#11149](nodejs#11149) * fs: allow WHATWG URL objects as paths (James M Snell) [nodejs#10739](nodejs#10739) * src: support UTF-8 in compiled-in JS source files (Ben Noordhuis) [nodejs#11129](nodejs#11129) * url: extend url.format to support WHATWG URL (James M Snell) [nodejs#10857](nodejs#10857) PR-URL: nodejs#11185

Updates the fs module APIs to allow 'file://' URL objects to be passed as the path. For example: ```js const URL = require('url').URL; const myURL = new URL('file:///C:/path/to/file'); fs.readFile(myURL, (err, data) => {}); ``` 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 ``` The documentation for the fs API is intentionally not updated in this commit because the URL API is still considered experimental and is not officially documented *at this time* Note that file: URLs are *required* by spec to always be absolute paths from the file system root. This is a semver-major commit because it changes error handling on the fs APIs. PR-URL: nodejs#10739 Ref: nodejs#10703 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Notable changes: * deps: * update V8 to 5.5 (Michaël Zasso) [#11029](nodejs/node#11029) * upgrade libuv to 1.11.0 (cjihrig) [#11094](nodejs/node#11094) * add node-inspect 1.10.4 (Jan Krems) [#10187](nodejs/node#10187) * upgrade zlib to 1.2.11 (Sam Roberts) [#10980](nodejs/node#10980) * lib: build `node inspect` into `node` (Anna Henningsen) [#10187](nodejs/node#10187) * crypto: Remove expired certs from CNNIC whitelist (Shigeki Ohtsu) [#9469](nodejs/node#9469) * inspector: add --inspect-brk (Josh Gavant) [#11149](nodejs/node#11149) * fs: allow WHATWG URL objects as paths (James M Snell) [#10739](nodejs/node#10739) * src: support UTF-8 in compiled-in JS source files (Ben Noordhuis) [#11129](nodejs/node#11129) * url: extend url.format to support WHATWG URL (James M Snell) [#10857](nodejs/node#10857) PR-URL: nodejs/node#11185 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

@addaleax said:

Should this come with docs changes?

but I didn't see any follow up comments. Why is this not documented? I just found it while browsing the fs source.

@sam-github Quoting the PR description (I think this had been added later):

The documentation for the fs API is intentionally not updated in this commit because the URL API is still considered experimental and is not officially documented at this time

I see. We can't document that fs APIs take a url.URL object when url.URL itself is undocumented. Fair enough.

To be fair, url.URL has been documented since this PR landed: https://nodejs.org/api/url.html#url_the_whatwg_url_api. I’ll open a good first contribution issue asking for docs for this feature.

jasnell added fs Issues and PRs related to the fs subsystem / file system. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Jan 11, 2017

nodejs-github-bot added dont-land-on-v4.x fs Issues and PRs related to the fs subsystem / file system. url Issues and PRs related to the legacy built-in url module. labels Jan 11, 2017

jasnell added semver-major PRs that contain breaking changes and should be released in the next major version. and removed dont-land-on-v4.x labels Jan 11, 2017

jasnell force-pushed the url-path branch from 1869b21 to 39dd306 Compare January 11, 2017 17:00

nodejs-github-bot added the dont-land-on-v7.x label Jan 11, 2017

jasnell removed url Issues and PRs related to the legacy built-in url module. dont-land-on-v7.x labels Jan 11, 2017

joyeecheung requested changes Jan 11, 2017

View reviewed changes

joyeecheung reviewed Jan 11, 2017

View reviewed changes

nodejs-github-bot added the dont-land-on-v7.x label Jan 11, 2017

jasnell force-pushed the url-path branch from c0dd5f6 to 5b530cc Compare January 11, 2017 18:37

nodejs-github-bot added the dont-land-on-v7.x label Jan 11, 2017

jasnell force-pushed the url-path branch from 5b530cc to 1260b09 Compare January 11, 2017 19:09

nodejs-github-bot added the dont-land-on-v7.x label Jan 11, 2017

jasnell removed the dont-land-on-v7.x label Jan 11, 2017

jasnell force-pushed the url-path branch from 1260b09 to 83a303c Compare January 12, 2017 00:56

addaleax mentioned this pull request Apr 11, 2017

doc,fs: document WHATWG URL support #12341

Closed

watilde mentioned this pull request Apr 22, 2017

not support file URLs containing drive specifiers (e.g., "file:///c:/path") #10703

Closed

gibfahn mentioned this pull request Apr 24, 2017

Potential Semver Minor Backports nodejs/Release#188

Closed

31 tasks

MylesBorins added dont-land-on-v4.x labels Apr 24, 2017

anchnk mentioned this pull request Apr 26, 2017

doc: add WHATWG file URLs in fs module #12670

Closed

2 tasks

TimothyGu mentioned this pull request May 8, 2017

file:// URLs node-fetch/node-fetch#75

Closed

addaleax mentioned this pull request Dec 13, 2017

Support url *strings* in fs APIs #17658

Closed

addaleax mentioned this pull request May 24, 2018

fs: allow file: URL strings as paths #20944

Closed

4 tasks

kevinkassimo mentioned this pull request Apr 23, 2019

fetch() a local file denoland/deno#2150

Closed

fs: allow WHATWG URL and file: URLs as paths by jasnell · Pull Request #10739 · nodejs/node · GitHub

Uh oh!

Uh oh!

fs: allow WHATWG URL and file: URLs as paths - #10739

fs: allow WHATWG URL and file: URLs as paths#10739jasnell wants to merge 1 commit intonodejs:masternodejs/node:masterfrom jasnell:url-pathjasnell/node:url-pathCopy head branch name to clipboard

Conversation

jasnell commented Jan 11, 2017 • edited by addaleax Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

jasnell commented Jan 11, 2017

Uh oh!

joyeecheung commented Jan 11, 2017

Uh oh!

jasnell commented Jan 11, 2017

Uh oh!

jasnell commented Jan 11, 2017

Uh oh!

joyeecheung Jan 11, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell Jan 11, 2017

Choose a reason for hiding this comment

Uh oh!

joyeecheung Jan 11, 2017

Choose a reason for hiding this comment

Uh oh!

joyeecheung Jan 11, 2017

Choose a reason for hiding this comment

Uh oh!

joyeecheung Jan 11, 2017

Choose a reason for hiding this comment

Uh oh!

joyeecheung Jan 11, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jan 11, 2017

Uh oh!

jasnell commented Jan 12, 2017

Uh oh!

sam-github commented Apr 11, 2017

Uh oh!

addaleax commented Apr 11, 2017

Uh oh!

sam-github commented Apr 11, 2017

Uh oh!

addaleax commented Apr 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

fs: allow WHATWG URL and file: URLs as paths#10739
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:url-path

jasnell commented Jan 11, 2017 •
edited by addaleax
Loading