url: expose pathToFileURL and fileURLToPath #22506

guybedford · 2018-08-24T16:30:29Z

As discussed in #22502 this exposes and documents url.pathToFileURL and url.fileURLToPath in order to assist with cross platform support and encoding consistency.

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

nodejs-github-bot · 2018-08-24T16:30:30Z

@guybedford build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/662/pipeline

bmeck · 2018-08-24T16:31:48Z

doc/api/url.md

+```js
+// BAD:
+// - fails in Windows
+// - doesn't handle loading paths using extended non-latin characters


not just that, also things like " " which are percent encoded

This isn't meant to be comprehensive, but we can certainly pad out the examples.

I expect this will also change over time, so I don't think we want to keep the docs exactly in sync with the code here either.

agree, but examples seem more prudent than statements like this which could be taken as the only exceptions thereby leading people only on unix and in latin charsets to feel this API is not needed

bmeck · 2018-08-24T16:35:24Z

lib/internal/url.js

  const tmp = new URL('file://');
  if (filepath.includes('%'))
    filepath = filepath.replace(percentRegEx, '%25');
  tmp.pathname = filepath;
  return tmp;
 }

+function toPathIfFileUrl(fileUrlOrPath) {
+  if (fileUrlOrPath == null || !fileUrlOrPath[searchParams] ||


why not instanceof URL?

This was what was already in place here, so I didn't want to alter it for fear of compatibility changes. We could certainly try instanceof.

i'm curious why it wasn't using instanceof, would prefer to use that if we expose it to users

Largely because of performance. Checking for the existence of the internal symbol is significantly faster than using instanceof

@jasnell I'm increasingly uncomfortable with stuff like this that is making things more loose and forgeable than needed. Do we have data on why this is faster, I was under the impression that newer V8s were optimizing out simple instanceof checks.

CC: @bmeurer

addaleax · 2018-08-24T16:33:55Z

doc/api/url.md

+## File URL Utility Functions
+
+When working with `file:///` URLs in Node.js (eg when working with ES modules
+which are keyed in the registry by File URL), the utility functions


I’d s/registry/module registry/ for clarity

module map, not registry

addaleax · 2018-08-24T16:34:35Z

doc/api/url.md

+```js
+// BAD:
+// - fails in Windows
+// - doesn't handle loading paths using extended non-latin characters


bmeck · 2018-08-24T16:36:28Z

would like to see more examples added to docs to emphasize why this is a good API to use. I left some at #22502 (comment)

including the fileURL in each example would help, be sure to uppercase the URL per https://url.spec.whatwg.org/#url-apis-elsewhere

guybedford · 2018-08-24T17:07:26Z

@bmeck thanks I've padded out the examples and renamed Url to URL.

guybedford · 2018-08-24T17:13:35Z

@vsemozhetbyt those other methods are provided under "legacy API", whereas these ones aren't part of the legacy API, but utilities that go alongside the WhatWG API.

guybedford · 2018-08-24T17:27:33Z

@vsemozhetbyt ah I missed that, sure I've combined it into the same section.

guybedford · 2018-08-24T18:38:37Z

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

vsemozhetbyt

Just some doc nits.

vsemozhetbyt · 2018-08-24T18:38:42Z

doc/api/url.md

+For example, the following errors can occur when converting from paths to URLs:
+
+```js
+// throws for missing schema (posix)


posix -> POSIX here and below?

vsemozhetbyt · 2018-08-24T18:44:09Z

doc/api/url.md

+new URL('./foo#1', 'file:///');
+
+// 'file:///nas/foo.txt' instead of the correct 'file:///foo.txt' (posix)
+new URL(`file://${'//nas/foo.txt'}`);


In this and the next example, template strings with string interpolation seem confusingly redundant. Maybe interpolate previously defined variables?

I quite like having these be scannable as single-line cases, initially I wrote new URL('file://' + '//nas/foo.txt') to indicate the separation, but the linter wouldn't allow that.

vsemozhetbyt · 2018-08-24T18:49:58Z

doc/api/url.md

+new URL(`file://${'//nas/foo.txt'}`);
+
+// 'file:///some/path%' instead of the correct 'file:///some/path%25' (posix)
+new URL(`sfile:${'/some/path%.js'}`);


sfile: -> file://

vsemozhetbyt · 2018-08-24T18:50:52Z

doc/api/url.md

+new URL(`sfile:${'/some/path%.js'}`);
+```
+
+where using `pathToFileURL` we can get the correct results above.


`pathToFileURL` -> `url.pathToFileURL()`

vsemozhetbyt · 2018-08-24T18:52:21Z

doc/api/url.md

+
+where using `pathToFileURL` we can get the correct results above.
+
+### url.fileURLToPath(url)


This section needs to go before ### url.format(URL[, options]) ABC-wise.

Sure, a shame we can't keep these together though.

vsemozhetbyt · 2018-08-24T18:53:01Z

doc/api/url.md

+
+### url.fileURLToPath(url)
+
+* `url` {URL} | {string} The file URL string or URL object to convert to a path.


{URL} | {string} -> {URL | string}

vsemozhetbyt · 2018-08-24T18:59:08Z

doc/api/url.md

+new URL('file:///hello world.txt').pathname;
+```
+
+where using `fileURLToPath` we can get the correct results above.


`fileURLToPath` -> `url.fileURLToPath()`

jdalton · 2018-08-24T19:16:18Z

lib/internal/url.js

+function toPathIfFileURL(fileURLOrPath) {
+  if (fileURLOrPath == null || !fileURLOrPath[searchParams] ||
+      !fileURLOrPath[searchParams][searchParams])
+    return fileURLOrPath;


What's the fileURLOrPath[searchParams][searchParams] check about?
The fileURLOrPath[searchParams] checks currently allow paths to duck-type as URLs.

This is discussed at #22506 (review).

Specifically asking about the double searchParams check of
fileURLOrPath[searchParams][searchParams].

This is exactly what is already implemented. We can switch to an instanceof, but would be good to hear from @jasnell if this would be compatible with the original logic.

I would prefer a brand check (i.e. [searchParams]) as it's already been used everywhere else where we are checking for a URL, for consistency if nothing else. The same logic would probably apply to fileURLToPath() as well.

Duck-typing isn't great. That it slipped in elsewhere in the project isn't a reason to continue it. It should really be reworked out when possible.

I managed to trace down the origin of this code, and it was a performance PR made here - #11690.

If we want to change this, we should do it in all of URL and not just this function, and as a separate PR.

a note, we shouldn't call forgable things "branding" generally. "tagging" is generally the term here (like Symbol.toStringTag)

TimothyGu

For this to be exposed to public, I believe we need to be more careful with input validation. For example, I'd rather have URL be the sole input for url.fileURLToPath(), and throw an exception otherwise.

guybedford · 2018-08-25T09:17:54Z

@TimothyGu what concerns do you have about input ambiguity here? Because URL strings are encouraged by the WhatWG spec (despite any performance concerns), I'm very much for functions permitting URL strings or URL objects equally. I think cases to worry about is string parsing where different cases of strings are permitted like with the fs functions, but as we have been doing allowing URL strings or objects equally seems to me a good pattern generally to avoid the performance issues while still allowing "supposed" best practices.

vsemozhetbyt · 2018-08-25T09:27:18Z

doc/api/url.md

+new URL('file:///hello world.txt').pathname;
+```
+
+where using `url.fileURLToPath` we can get the correct results above.


We usually add parentheses to functions/methods as per the style guide, so:
`url.fileURLToPath` -> `url.fileURLToPath()`

vsemozhetbyt · 2018-08-25T09:27:34Z

doc/api/url.md

 ```

-where using `fileURLToPath` we can get the correct results above.
+where using `url.pathToFileURL` we can get the correct results above.


PR-URL: #22506 Reviewed-By: John-David Dalton <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Bradley Farias <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gus Caplan <[email protected]>

Notable changes: * fs * Added a `recursive` option to `fs.mkdir` and `fs.mkdirSync`. If this option is set to `true`, non-existing parent folders will be automatically created. #21875 * Fixed fsPromises.readdir `withFileTypes`. #22832 * http2 * Added `http2stream.endAfterHeaders` property. #22843 * module * Added `module.createRequireFromPath(filename)`. This new method can be used to create a custom `require` function that will resolve modules relative to the `filename` path. #19360 * url * Added `url.fileURLToPath(url)` and `url.pathToFileURL(path)`. These methods can be used to correctly convert between `file:` URLs and absolute paths. #22506 * util * Added `util.types.isBoxedPrimitive(value)`. #22620 * Added new collaborators: * boneskull (https://github.com/boneskull) - Christopher Hiller * The Technical Steering Committee has new members: * apapirovski (https://github.com/apapirovski) - Anatoli Papirovski * gabrielschulhof (https://github.com/gabrielschulhof) - Gabriel Schulhof PR-URL: #22932

targos · 2018-09-20T05:55:02Z

Depends on #21875. Marking dont-land-on-v10.x for now.

PR-URL: #22506 Reviewed-By: John-David Dalton <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Bradley Farias <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gus Caplan <[email protected]>

Notable changes: * assert * The diff output is now a tiny bit improved by sorting object properties when inspecting the values that are compared with each other. #22788 * cli * The options parser now normalizes `_` to `-` in all multi-word command-line flags, e.g. `--no_warnings` has the same effect as `--no-warnings`. #23020 * Added bash completion for the `node` binary. To generate a bash completion script, run `node --completion-bash`. The output can be saved to a file which can be sourced to enable completion. #20713 * crypto * Added support for PEM-level encryption. #23151 * Added an API asymmetric key pair generation. The new methods `crypto.generateKeyPair` and `crypto.generateKeyPairSync` can be used to generate public and private key pairs. The API supports RSA, DSA and EC and a variety of key encodings (both PEM and DER). #22660 * fs * Added a `recursive` option to `fs.mkdir` and `fs.mkdirSync`. If this option is set to true, non-existing parent folders will be automatically created. #21875 * http2 * Added a `'ping'` event to `Http2Session` that is emitted whenever a non-ack `PING` is received. #23009 * Added support for the `ORIGIN` frame. #22956 * module * Added `module.createRequireFromPath(filename)`. This new method can be used to create a custom require function that will resolve modules relative to the filename path. #19360 * process * Added a `'multipleResolves'` process event that is emitted whenever a `Promise` is attempted to be resolved multiple times, e.g. if the `resolve` and `reject` functions are both called in a `Promise` executor. #22218 * **url** * Added `url.fileURLToPath(url)` and `url.pathToFileURL(path)`. These methods can be used to correctly convert between file: URLs and absolute paths. #22506 * **util** * Added the `sorted` option to `util.inspect()`. If set to `true`, all properties of an object and Set and Map entries will be sorted in the returned string. If set to a function, it is used as a compare function. #22788 * The `util.instpect.custom` symbol is now defined in the global symbol registry as `Symbol.for('nodejs.util.inspect.custom')`. #20857 * **Windows** * The Windows msi installer now provides an option to automatically install the tools required to build native modules. #22645 * **Added new collaborators**: * digitalinfinity - Hitesh Kanwathirtha PR-URL: #23313

Notable changes: * assert * The diff output is now a tiny bit improved by sorting object properties when inspecting the values that are compared with each other. #22788 * cli * The options parser now normalizes `_` to `-` in all multi-word command-line flags, e.g. `--no_warnings` has the same effect as `--no-warnings`. #23020 * Added bash completion for the `node` binary. To generate a bash completion script, run `node --completion-bash`. The output can be saved to a file which can be sourced to enable completion. #20713 * crypto * Added support for PEM-level encryption. #23151 * Added an API asymmetric key pair generation. The new methods `crypto.generateKeyPair` and `crypto.generateKeyPairSync` can be used to generate public and private key pairs. The API supports RSA, DSA and EC and a variety of key encodings (both PEM and DER). #22660 * fs * Added a `recursive` option to `fs.mkdir` and `fs.mkdirSync`. If this option is set to true, non-existing parent folders will be automatically created. #21875 * http2 * Added a `'ping'` event to `Http2Session` that is emitted whenever a non-ack `PING` is received. #23009 * Added support for the `ORIGIN` frame. #22956 * Updated nghttp2 to 1.34.0. This adds RFC 8441 extended connect protocol support to allow use of WebSockets over HTTP/2. #23284 * module * Added `module.createRequireFromPath(filename)`. This new method can be used to create a custom require function that will resolve modules relative to the filename path. #19360 * process * Added a `'multipleResolves'` process event that is emitted whenever a `Promise` is attempted to be resolved multiple times, e.g. if the `resolve` and `reject` functions are both called in a `Promise` executor. #22218 * url * Added `url.fileURLToPath(url)` and `url.pathToFileURL(path)`. These methods can be used to correctly convert between file: URLs and absolute paths. #22506 * util * Added the `sorted` option to `util.inspect()`. If set to `true`, all properties of an object and Set and Map entries will be sorted in the returned string. If set to a function, it is used as a compare function. #22788 * The `util.instpect.custom` symbol is now defined in the global symbol registry as `Symbol.for('nodejs.util.inspect.custom')`. #20857 * Added support for `BigInt` numbers in `util.format()`. #22097 * V8 API * A number of V8 C++ APIs have been marked as deprecated since they have been removed in the upstream repository. Replacement APIs are added where necessary. #23159 * Windows * The Windows msi installer now provides an option to automatically install the tools required to build native modules. #22645 * Workers * Debugging support for Workers using the DevTools protocol has been implemented. #21364 * The public `inspector` module is now enabled in Workers. #22769 * Added new collaborators: * digitalinfinity - Hitesh Kanwathirtha PR-URL: #23313

url: expose pathToFileUrl and fileUrlToPath

4d95ca1

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Aug 24, 2018

bmeck reviewed Aug 24, 2018

View reviewed changes

addaleax reviewed Aug 24, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

Url -> URL, pad out examples

0d9c808

guybedford changed the title ~~url: expose pathToFileUrl and fileUrlToPath~~ url: expose pathToFileURL and fileURLToPath Aug 24, 2018

guybedford added 2 commits August 24, 2018 19:09

update windows examples

0b784b4

correction

6479897

guybedford force-pushed the url-utils branch from daba4e5 to 6479897 Compare August 24, 2018 17:12

registry -> module map

09bc85a

This comment has been minimized.

Sign in to view

document utils in same section

c073384

addaleax added semver-minor PRs that contain new features and should be released in the next minor version. whatwg-url Issues and PRs related to the WHATWG URL implementation. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Aug 24, 2018

vsemozhetbyt reviewed Aug 24, 2018

View reviewed changes

jdalton reviewed Aug 24, 2018

View reviewed changes

TimothyGu reviewed Aug 24, 2018

View reviewed changes

readme corrections

d36d1f9

vsemozhetbyt reviewed Aug 25, 2018

View reviewed changes

style nits

47d9efa

targos added the dont-land-on-v10.x label Sep 20, 2018

targos mentioned this pull request Sep 20, 2018

fs: implement recursive (mkdirp) functionality #21875

Closed

4 tasks

targos removed the dont-land-on-v10.x label Sep 25, 2018

guybedford mentioned this pull request Oct 2, 2018

Discussion: File URLs in Node.js #22502

Closed

targos mentioned this pull request Oct 7, 2018

Release proposal: v10.12.0 #23313

Merged

jdalton mentioned this pull request Dec 31, 2018

fileURLToPath returns forward slashes on Windows #25265

Closed


		where using `pathToFileURL` we can get the correct results above.

		### url.fileURLToPath(url)


		### url.fileURLToPath(url)

		* `url` {URL} \| {string} The file URL string or URL object to convert to a path.

url: expose pathToFileURL and fileURLToPath #22506

url: expose pathToFileURL and fileURLToPath #22506

Conversation

guybedford commented Aug 24, 2018 • edited Loading

Checklist

nodejs-github-bot commented Aug 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmeck commented Aug 24, 2018

This comment has been minimized.

guybedford commented Aug 24, 2018

guybedford commented Aug 24, 2018

This comment has been minimized.

guybedford commented Aug 24, 2018

guybedford commented Aug 24, 2018

vsemozhetbyt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu left a comment

Choose a reason for hiding this comment

guybedford commented Aug 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos commented Sep 20, 2018

guybedford commented Aug 24, 2018 •

edited

Loading