Skip to content
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

Merged
merged 8 commits into from
Jun 29, 2020

Conversation

teikjun
Copy link
Contributor

@teikjun teikjun commented Jun 29, 2020

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

  1. Follow the steps for reproducing client-redirects plugin should not be baseUrl sensitive #2982
  2. The redirection is working now

Test D2 website client-redirect

  1. Go to https://deploy-preview-3010--docusaurus-2.netlify.app/docs/introduction
  2. You should be redirected to /docs

@teikjun teikjun requested a review from yangshun as a code owner June 29, 2020 10:35
@teikjun teikjun changed the title fix(v2): make client-redirect-plugin not baseUrl-sensitive fix(v2): make client-redirect-plugin not baseUrl sensitive Jun 29, 2020
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jun 29, 2020
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Jun 29, 2020

Deploy preview for docusaurus-2 ready!

Built with commit 727ebea

https://deploy-preview-3010--docusaurus-2.netlify.app

@slorber slorber added pr: bug fix This PR fixes a bug in a past release. v2 labels Jun 29, 2020
Copy link
Collaborator

@slorber slorber left a 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 {
Copy link
Collaborator

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);
Copy link
Collaborator

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

@slorber
Copy link
Collaborator

slorber commented Jun 29, 2020

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[] {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

??? :p

Copy link
Contributor Author

@teikjun teikjun Jun 29, 2020

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

Copy link
Collaborator

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

Copy link
Collaborator

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

Copy link
Contributor Author

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 :)

@teikjun teikjun requested a review from slorber June 29, 2020 14:49
@slorber slorber merged commit 2e055f4 into facebook:master Jun 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

client-redirects plugin should not be baseUrl sensitive
4 participants