-
-
Notifications
You must be signed in to change notification settings - Fork 8.5k
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(v2): fix redirect toUrl (windows + trailing slash) #3903
fix(v2): fix redirect toUrl (windows + trailing slash) #3903
Conversation
Size Change: +15 B (0%) Total Size: 154 kB ℹ️ View Unchanged
|
✔️ Deploy preview for docusaurus-2 ready! 🔨 Explore the source changes: 127c3bb 🔍 Inspect the deploy logs: https://app.netlify.com/sites/docusaurus-2/deploys/5fe9b6a10e03ee0008f1b436 😎 Browse the preview: https://deploy-preview-3903--docusaurus-2.netlify.app |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3903--docusaurus-2.netlify.app/classic/ |
@@ -42,7 +42,7 @@ export function toRedirectFilesMetadata( | |||
); | |||
const toUrl = addTrailingSlash( | |||
`${removeTrailingSlash(pluginContext.baseUrl)}${path.join(redirect.to)}`, | |||
); | |||
).replace(/\\/g, '/'); |
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.
Hi and thanks for working on this issue.
I have a hard time understanding where the '\' comes from, can you log the baseurl and redirect.to params so that we figure this out?
I feel like your solution works but is more a quickfix, and we should rather fix the problem at its root, where the \ is used instead of /
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.
Can you try running this code on your computer?
const path = require("path");
function addTrailingSlash(str) {
return str.endsWith('/') ? str : `${str}/`;
}
function removeTrailingSlash(str) {
return removeSuffix(str, '/');
}
function removeSuffix(str, suffix) {
if (suffix === '') {
return str; // always returns "" otherwise!
}
return str.endsWith(suffix) ? str.slice(0, -suffix.length) : str;
}
//////////////////////////////////////////
// test code
const baseUrl = "/";
const to = '/docs/something/else';
const toUrl = addTrailingSlash(
`${removeTrailingSlash(baseUrl)}${path.join(to)}`
);
console.log(toUrl);
I suspect the path.join(to)
is what adds the \\
, it does not seem correct to use it to construct a URL path.
The weird thing is path.join(to)
does nothing for me as there's only 1 param, but maybe the behavior on Windows is different?
We could replace it with the normalizeUrl([baseUrl,to])
in @docusaurus/utils
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.
Should be able to get this fixed thanks to CI and windows tests, FYI I'm pushing to your branch so that you are credited for your contribution
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, unfortunately forgot that our Jest setup on windows is still not good 😓 (#3540)
I believe this "normalizeUrl" could fix the issue, so going to merge but please let me know if you still have the problem after this change I made to your PR
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.
@slorber Glad to help.
The \
is defiantly coming from the path.join(to)
. Windows is special and uses \
for it's path separator unlike any other OS.
Here are the results from the test code above.
On Windows, I get \docs\something\else/
.
On WSL I get the expected output /docs/something/else/
.
I do agree that path.join(to)
is probably the best way to to construct URLs, but I did find path.posix.join(to)
produces the expected output on Windows.
do not add trailing slash + try to fix windows path issues
Motivation
@docusaurs-plugin-client-redirects fails on windows because windows is special and uses \ in path instead of /.
Replacing \ with / before url encoding resolves this issue.
Resolves #3886
Have you read the Contributing Guidelines on pull requests?
Yes