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

Leading slashes in paths on Windows #14605

Closed
0kku opened this issue May 14, 2022 · 12 comments
Closed

Leading slashes in paths on Windows #14605

0kku opened this issue May 14, 2022 · 12 comments

Comments

@0kku
Copy link

0kku commented May 14, 2022

Running the following will log the strings described in the comments:

console.log(
  import.meta.url, // "file:///D:/test/testing.ts"
  new URL(import.meta.url).pathname, // "/D:/test/testing.ts"
);

This is spec-compliant behaviour and not a bug.

However, this causes some cross-platform issues with code like:

console.log(await Deno.readTextFile(new URL(import.meta.url).pathname)); // path has the leading slash

(reading the file itself as text). Obviously, this is a contrived example, but I would expect it to work. And it does work on Mac. A more reasonable example might look like this:

import { dirname, join } from "https://deno.land/std@0.139.0/path/win32.ts";

const path = join(dirname(new URL(import.meta.url).pathname), "units.txt"); // path has the leading slash
console.log(await Deno.readTextFile(path));

I would expect this to work on all platforms, but on Windows, both of the above examples result in error: Uncaught (in promise) NotFound: The system cannot find the file specified. (os error 2).


Since leaving the leading slash in is spec-compliant behaviour for URL.pathname, I propose that Deno ought to ignore the leading slash in file paths to ensure that code is portable.

@bartlomieju
Copy link
Member

bartlomieju commented May 14, 2022

Instead of:

console.log(await Deno.readTextFile(new URL(import.meta.url).pathname)); // path has the leading slash

do:

console.log(await Deno.readTextFile(new URL(import.meta.url)));

Most of Deno APIs that support filepaths, will also handle URLs.

@crowlKats
Copy link
Member

crowlKats commented May 14, 2022

@bartlomieju the problem with that is that any needed modification with that dirname & join isnt doable. maybe dirname & join (and others) should maybe support accepting a URL?

@andreubotella
Copy link
Contributor

andreubotella commented May 14, 2022

new URL(...).pathname has a first slash per the spec. If you need to turn a URL into a OS-valid path, why not use fromFileUrl from https://deno.land/std@0.139.0/path/mod.ts

@bartlomieju
Copy link
Member

@bartlomieju the problem with that is that any needed modification with that dirname & join isnt doable. maybe dirname & join (and others) should maybe support accepting a URL?

Depends, you can always use fromFileUrl() first

@nayeemrmn
Copy link
Collaborator

@bartlomieju the problem with that is that any needed modification with that dirname & join isnt doable

Correct, that's why instead of those you have to use the URL constructor (e.g. new URL(".", url) for dirname) otherwise you have to go full paths with fromFileUrl(). Url::pathname don't substitute FS paths

@crowlKats
Copy link
Member

you three are right, completely forgot about fromFileUrl was a thing

@0kku
Copy link
Author

0kku commented May 14, 2022

join(dirname(new URL(fromFileUrl(import.meta.url)).pathname), "units.txt")

does work, yes, but I think it's a bit concerning that it's possible to so trivially write code that works on unix systems and doesn't work on Windows (and vice versa). I cloned a Deno project from GitHub, and tried to run it, but it didn't work on Windows. It's a bit disappointing if Deno promises cross-platform portable code, yet a project might fail to run on only some systems. This doesn't feel like something that ought to be OS-dependent imo, and the proposed solution feels more like a workaround.

@nayeemrmn
Copy link
Collaborator

join(dirname(new URL(fromFileUrl(import.meta.url)).pathname), "units.txt")

The proper way of writing this is:

new URL("units.txt", import.meta.url)

The URL constructor will pretty much always satisfy these use cases. In fact, forget about fromFileUrl().

@bartlomieju
Copy link
Member

I cloned a Deno project from GitHub, and tried to run it, but it didn't work on Windows. It's a bit disappointing if Deno promises cross-platform portable code, yet a project might fail to run on only some systems. This doesn't feel like something that ought to be OS-dependent imo, and the proposed solution feels more like a workaround.

Exactly for this reason the FS APIs accept URLs so you don't have to do the conversion yourself.

@0kku
Copy link
Author

0kku commented May 14, 2022

In that case it might be nice if they only accepted URLs so that people wouldn't accidentally write code that might or might not run depending on the OS it happens to be run on?

To elaborate: the issue I'm reporting is that it's possible to accidentally write non-portable code, not that it isn't possible to get it to work on all OSs when written correctly. The compiler shouldn't let people write non-portable code in the first place in situations like these.

@andreubotella
Copy link
Contributor

To elaborate: the issue I'm reporting is that it's possible to accidentally write non-portable code, not that it isn't possible to get it to work on all OSs when written correctly. The compiler shouldn't let people write non-portable code in the first place in situations like these.

That would be a breaking change, and I don't think that would be changed until Deno 2.0 (if it ever really comes).

@lucacasonato
Copy link
Member

lucacasonato commented May 14, 2022

URLs can not represent all valid UNIX file paths, as such we must support accepting both URL and string. This behavior is consistent with Node and the URL spec, so I think there is nothing actionable for us to do here, other than writing more docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants