-
Notifications
You must be signed in to change notification settings - Fork 43
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
refactor(joinURL): rewrite with clear syntax and relative ../
support
#218
Conversation
test/join.test.ts
Outdated
// Relative with ../ | ||
{ input: ["/a", "../b"], out: "/b" }, | ||
{ input: ["/a/b/c", "../../d"], out: "/a/d" }, | ||
{ input: ["/c", "../../d"], out: "/d" }, |
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.
Updated behavior from #217 (was ../d
). Confirming with two similar references:
new URL("../../d", 'http://test.com/c').pathname
// ~> "/d"
path.join("/c", "../../d")
// ~> "/d"
pathe.join("/c", "../../d")
// ~> "/d"
It is hard to decide if we should make a new 3rd behavior with ufo
(which is really hard to break later)
ufo
(1.4.0) behavior:
ufo.joinURL("/c", "../../d")
// "/c/../../d"
(with current behavior, we don't resolve, nor loose information but also we don't have a 3rd behavior by returning ../d
)
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.
I think existing behaviour is not to coerce a URL segment to be absolute, so joinURL('./a', 'b')
resolves to ./a/b
. Likewise, I think joinURL('../a', 'b')
should also resolve to ../a/b
and joinURL('../a', '../b')
should resolve to ../b
.
The key difference between new URL
and joinURL
is that joinURL
is useable for URL segments and this is quite a useful feature. It's also important when generating relative URLs, which is something that the native URL
can't do.
I would advise keeping this behaviour.
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.
I still think we should allow 'escaping' out of /
if the first segment has /
and subsequent segments have ../
.
But that shouldn't affect the Nuxt usage.
Really appreciate your work on this ❤️
As a continuation to #217 (to resolve #37), this PR reimplements
joinURL
utility with more clear syntax and basic support for relative../
segmentsBehavior change: For
joinURL
andwithBase
, the trailing slash of base is always respected. It is more of a bug fix.