Support url *strings* in fs APIs

[jkrems]

• Version: 7.6.0+
• Platform: all
• Subsystem: fs

Right now the FS APIs do support URLs - but only if they are passed as URL objects. This seems to go against the use of URLs in DOM APIs where methods tend to accept strings. E.g. fetch only "accepts" URL objects because they happen to stringify to their .href property.

Would it be acceptable to extend the current support for file URLs to "any string starting with file:// will be treated like a URL object"? This would remove the need for people to manually create import and then instantiate a URL object if they already have a valid file URL (e.g. via import.meta.url).

[TimothyGu]

I'm fine with it if it's shown that performance of fs APIs given regular path strings does not decrease because of this. Checking for a URL object is much simpler than checking for the first 7 characters, though I agree that having to reparse URLs is a hassle. I also understand that accepting URL objects is not recommended by the URL Standard in the first place.

[addaleax]

We discussed this in the original PR a bit (at first, it contained support for that): #10739 (comment)

[jkrems]

It looks like it was backed out of the original PR for security concerns. Will try to assemble that background once I’m back at a real computer.

…

On Wed, Dec 13, 2017 at 10:11 AM Timothy Gu ***@***.***> wrote: I'm fine with it if it's shown that performance of fs APIs given regular path strings does not decrease because of this. Checking for a URL object is much simpler than checking for the first 7 characters, though I agree that having to reparse URLs is a hassle. I also understand that accepting URL objects is not recommended <https://url.spec.whatwg.org/#url-apis-elsewhere> by the URL Standard in the first place. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#17658 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAio9AcnF0AqTJEeWcrpjCuq4CGTeZqtks5tABNZgaJpZM4RA83e> .

[jasnell]

There are a couple of issues here:

1. Allowing URL strings means we would have to parse all string inputs through new URL(), which is definite performance hit.
2. It also means that we risk breaking existing cases where file names have a : in them

At this point I'm -1 on modifying the existing APIs further to support this case, tho I would not be opposed to alternate methods that support it. e.g., instead of fs.readFile('file://whatever'), something like a fs.ureadFile('file://whatever'). This would follow the pattern established for methods that accept `fd's rather than paths, so there is precedent.

[jkrems]

Interesting. I agree that continuously overloading the meaning of the existing APIs isn't great. Originally I had something like "only if the string starts exactly with 'file://'" which might address the performance or existing filename concerns. Still, leaves the security concerns and a potentially confusing API ("here's the 20 different things that might be passed in as a filename").

I could see fs.u* or a similar convention for URL-only file system methods. Super tempted to suggest making them async/promise-returning from the start. Not sure if that would make it a lot harder to add.

[jasnell]

I've already got an effort under way to work up some Promise-based fs APIs. The PR has been sitting there for a bit without updating, but it will be advancing. There are definitely going to be some differences between it and the current API so I'm weighing just how much different it should be.

[jkrems]

Thanks, I'll have a look at #15485!

[pauldraper]

The real issue IMO, is that this is simply incorrect.

Files may contain : on many file systems, and repeated slashes are permitted in paths.

If it's a string, it should be treated as a path. If it's a URL, it should be treated as a URL.

[TimothyGu]

@pauldraper's comment is a dealbreaker for me:

$ mkdir -p file\:/abc
$ echo 'hello world' > file\:/abc/abc
$ node
> fs.readFileSync('file:///abc/abc', 'utf8')
'hello world\n'

I'm going to close this as something that can't reasonably be accomplished.