fix: When alias contains parameters, append ext error#1855
fix: When alias contains parameters, append ext error#1855sy-records merged 7 commits intodevelopfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 1c5a0cb:
|
src/core/router/history/base.js
Outdated
| : `${path}${ext}`; | ||
| : path.indexOf('?') === -1 | ||
| ? `${path}${ext}` | ||
| : path; |
There was a problem hiding this comment.
We need to handle all URLs the same with or without the query, just like regular HTML in a browser.
For example, /path/to/folder/?foo=123 has a query, and we still need to look for /path/to/folder/README.md?foo=123, but this skips anything with a query.
In the end, we need to operate on the path separately, and always send the query with the final result.
There was a problem hiding this comment.
@trusktr, is there anything that could help push this along? I'm not that familiar with js but I am good with regular expressions. If you can provide a little more context on the issue perhaps I could put a little time into it, as fixing this would open up a lot of doors for those of us needing to federate markdown files from private repos/orgs on GitHub.
There was a problem hiding this comment.
@vergilcw sorry for late reply. I think the solution should determine the fully qualified URL (f.e. anchor.href = partialUrlString; const fullyQualifiedUrl = anchor.href) and then use a new URL object to get all the pieces and use them correctly.
There was a problem hiding this comment.
I think the solution should determine the fully qualified URL (f.e.
anchor.href = partialUrlString; const fullyQualifiedUrl = anchor.href) and then use a new URL object to get all the pieces and use them correctly.
Have a chance to try this out?
There was a problem hiding this comment.
Can you clarify the difference between using new URL and the changes in this PR?
There was a problem hiding this comment.
In most cases, path is an Invalid URL, so I don't use it.
trusktr
left a comment
trusktr
left a comment
There was a problem hiding this comment.
See comment. Basically this still has other problems. The solution is to handle paths and queries separately
|
@SumithChandran hi, sorry for taking long. Did you find a solution? I commented on a direction we should look at. We just need to handle all the URL parts correctly, f.e. get all the parts of the URL, do what we need with only the path part, then put everything back together properly. |
@trusktr , no I haven't found anything solution. |
|
Any chance this will get done soon? I've actually never committed to a public repo, but this looks pretty straightforward. I'd be willing to try. |
@zomgbre willing to take a stab at it? We haven't circled back yet |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
6 participants
Summary
As mentioned in #1854, if the URL used by the alias contains parameters, we will add the 'ext' configuration item incorrectly
What kind of change does this PR introduce?
Bugfix
For any code change,
Does this PR introduce a breaking change? (check one)
If yes, please describe the impact and migration path for existing applications:
Related issue, if any:
Fix #1854
Tested in the following browsers: