-
Notifications
You must be signed in to change notification settings - Fork 156
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
Fix #129: handle relative paths on links and route calls #243
base: main
Are you sure you want to change the base?
Conversation
src/index.js
Outdated
a.setAttribute('href', url); | ||
url = a.href; | ||
} | ||
let [,protocol,host,pathname,search,hash] = uriRegex.exec(url); |
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.
Could we skip the parse entirely and just do this?
return a.pathname + a.search + a.hash;
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 may have been worrying about server-side (nodejs) rendering where we don't have an a
to do URL resolution: is that a valid concern?
If not, I'll happily simplify.
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 we can probably skip the SSR concern for now - route() should be a no-op there anyway since SSR is single-pass.
@developit Any plans to merge this? |
@developit can i help for this PR ? I need it badly 😅 |
|
||
// attempt to route, if no match simply cede control to browser | ||
return route(href); | ||
return route(node.pathname + node.search + node.hash); |
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 this could use node.href
.
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.
It's been a while since I wrote this, but think the reason I did this was for some sort of consistency with getCurrentUrl
(which omits protocol/host/port)
Reimplement #138 with clean commit history, but bringing in changes from #177 too.