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

url: fix toPathIfFileUrl with string URL input #49466

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AidanWelch
Copy link

Functions using toPathIfFileURL (such as fs.readdir) would have unexpected behavior when a string of a file URL was input such as import.meta.url, fileURLToPath supports string input so there is no reason to not convert file URL strings.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/url

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Sep 3, 2023
Copy link
Contributor

@LiviaMedeiros LiviaMedeiros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AidanWelch thank you for pull request!
However, I don't think we can change this, see #48994 for reference.

To use URLs with node:fs, user should pass URL object so it can be distinguished from path. For example, to read contents of directory where current file is located, use readdir(new URL('.', import.meta.url)) rather than readdir(new URL('.', import.meta.url).href).

If this looks unexpected, maybe you would want to update File URL paths part of documentation instead to explain it more clearly?

@AidanWelch
Copy link
Author

I think it is unexpected because import actually does support URL strings, furthermore the conversion for URL to Path also supports URL strings. I don't see the point in the ambiguity of sometimes assuming string inputs are always paths and sometimes not, especially when it is a very trivial check,

@LiviaMedeiros
Copy link
Contributor

import, url.fileURLToPath (and some other APIs such as Fetch API) accept URL but do not accept paths, hence there is no ambiguity for them.

Any API that uses toPathIfFileURL() function accepts paths as string and URLs as object. Urlstring can be interpreted as perfectly valid path that points on different location, so unless we drop pathstring support first, we can't accept urlstrings.

@AidanWelch
Copy link
Author

Urlstring can be interpreted as perfectly valid path that points on different location, so unless we drop pathstring support first, we can't accept urlstrings.

How so?

@aduh95
Copy link
Contributor

aduh95 commented Sep 3, 2023

Urlstring can be interpreted as perfectly valid path that points on different location, so unless we drop pathstring support first, we can't accept urlstrings.

How so?

file:///dev/null can be parsed either as a relative path leading to ./file:/dev/null, or as an absolute URL leading to /dev/null. Node.js interprets it as the former for all the APIs that accept paths (e.g. fs.readdir), and as the latter for APIs that do not accept paths (e.g. fileURLToPath).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants