-
Notifications
You must be signed in to change notification settings - Fork 629
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
feat(path/unstable): support URL
in first arg of join()
#5863
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5863 +/- ##
==========================================
+ Coverage 96.26% 96.34% +0.07%
==========================================
Files 479 479
Lines 38705 38674 -31
Branches 5617 5631 +14
==========================================
+ Hits 37259 37260 +1
+ Misses 1403 1370 -33
- Partials 43 44 +1 ☔ View full report in Codecov by Sentry. |
URL
in first arg of join
|
||
// URLs | ||
[[new URL("file:///"), "x/b", "..", "/b/c.js"], "/x/b/c.js"], | ||
[[new URL("file:///foo"), "../../../bar"], "/bar"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first test case looks good. But I'd expect the result of the 2nd test case to be "/../../bar"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/bar
is the normalized form of /../../bar
. There's no parent directory of the root directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I tested with node:path
and that does appear to be the case. That doesn't really make sense to me but I guess is fine 🤷🏾♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did you test it? The below prints /bar
both in Node.js and Deno
import { join } from "node:path";
console.log(join("/foo", "../../../bar"));
BTW join("file:///foo", "../../../bar")
becomes ../bar
because "file:///foo"
is considered relative path file:/bar
(file:
part is considered as directory name instead of scheme name)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current tests seem a little bare. Could we please add the tests from the examples?
URL
in first arg of join
URL
in first arg of join()
part of #5537
This PR adds support of URL as the first parameter of
join