-
-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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): make client-redirect-plugin not baseUrl sensitive #3010
fix(v2): make client-redirect-plugin not baseUrl sensitive #3010
Conversation
Deploy preview for docusaurus-2 ready! Built with commit 727ebea |
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.
LGTM, just create a more generic "removePrefix" utils
@@ -43,3 +45,7 @@ export default function pluginClientRedirectsPages( | |||
}, | |||
}; | |||
} | |||
|
|||
export function trimBaseUrl(path: string, baseUrl: string): string { |
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.
we already have removeSuffix in docusaurus-utils, I'd suggest adding removePrefix in this package instead as this kind of function is often useful in multiple places
formattedError = unknownFields | ||
? `${formattedError}These field(s) [${unknownFields}] are not recognized in ${CONFIG_FILE_NAME}.\nIf you still want these fields to be in your configuration, put them in the 'customFields' attribute.\nSee https://v2.docusaurus.io/docs/docusaurus.config.js/#customfields` | ||
: formattedError; | ||
throw new Error(formattedError); |
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.
not really related, next time make a other PR for these changes
seems to work fine I'm able to redirect http://localhost:5000/build/docs/introduction to http://localhost:5000/build/docs locally |
@@ -46,6 +45,6 @@ export default function pluginClientRedirectsPages( | |||
}; | |||
} | |||
|
|||
export function trimBaseUrl(path: string, baseUrl: string): string { | |||
return path.startsWith(baseUrl) ? path.replace(baseUrl, '/') : path; | |||
export function trimBaseUrls(paths: string[], baseUrl: string): string[] { |
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.
??? :p
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 was hesitant to inline this method because it looked messy 🤣
It's inline now! :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.
@teikjun actually I agree with you to not inline and write a test (using the leading / is not so obvious), it's just you didn't use the removePrefix method :p
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.
but that's not a big deal, I think we can merge as is
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.
Ohh, I see! I'll refactor it next time if I work on this part of the code again :)
Motivation
This PR makes client-redirect-plugin not baseUrl sensitive by using relative paths.
It also fixes a minor client-redirect issue in the Docusaurus website.
This PR closes #2982.
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Test client-redirect-plugins with baseUrl
Test D2 website client-redirect
/docs