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): normalize location for route matching #2393

Merged
merged 4 commits into from
Mar 23, 2020

Conversation

rlamana
Copy link
Contributor

@rlamana rlamana commented Mar 9, 2020

Motivation

Fixes #2392

Have you read the Contributing Guidelines on pull requests?

Yes.

Test Plan

The PendingNavigation component now uses a normalizeLocation function. I added a single file for the new function to easily add unit tests and check the proper normalization of the pathname in the location object before it is passed to react-router's Route components for matching:

Original Path Normalized Path
/docs/introduction/index.html /docs/introduction
/docs/introduction /docs/introduction
/docs/introduction/foo.html /docs/introduction/foo.html
/index.html /
/ /

@rlamana rlamana requested a review from yangshun as a code owner March 9, 2020 17:35
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Mar 9, 2020
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Mar 9, 2020

Deploy preview for docusaurus-2 ready!

Built with commit 203adee

https://deploy-preview-2393--docusaurus-2.netlify.com

@rlamana
Copy link
Contributor Author

rlamana commented Mar 10, 2020

Just to add more information about this change: since react-router-config.renderRoutes() is being used, we can use the root <Route> to pass a normalized location object to all its Route descendants and force the match for locations that have index.html.

A location with pathname=/docs/introduction/index.html is transformed into pathname=/docs/introduction and passed for matching to the generated routes.

@rlamana rlamana changed the title Normalize location before passing route to react-router Normalize location before route matching Mar 10, 2020
@rlamana rlamana changed the title Normalize location before route matching Normalize location for route matching Mar 11, 2020
@rlamana rlamana changed the title Normalize location for route matching fix(v2): Normalize location for route matching Mar 11, 2020
Copy link
Contributor

@lex111 lex111 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@yangshun yangshun changed the title fix(v2): Normalize location for route matching fix(v2): normalize location for route matching Mar 21, 2020
Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

Why do we need the location to be part of the state now? I think we just have to normalize it before using it and it should be fine? Putting in state seems redundant.

@rlamana
Copy link
Contributor Author

rlamana commented Mar 21, 2020

@yangshun the normalized location is also used in the preload and startProgressBar functions. Even though the call to normalize is very, very simple, it seemed like a good optimization to just call it once when the location changes, instead of calling it multiple times. Since its value affects rendering, the right place for it would be the component's state.

Let me know what you think, I can always change the PR to call normalize whenever is needed instead of using the state.

@yangshun
Copy link
Contributor

@rlamana I would prefer to keep things stateless if possible if they can be derived. You could memoize the values instead of putting them inside state.

@rlamana
Copy link
Contributor Author

rlamana commented Mar 23, 2020

@yangshun makes sense. I updated the PR to memoize the normalizeLocation function and cache the already calculated paths.

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

Please address comment.

@@ -94,7 +96,7 @@ class PendingNavigation extends React.Component {
this.clearProgressBarTimeout();
this.progressBarTimeout = setTimeout(() => {
clientLifecyclesDispatcher.onRouteUpdateDelayed({
location: this.props.location,
location: this.state.location,
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no this.state.location

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😅 I missed a reference to the old state value... Now it's fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you! 😄

@yangshun yangshun added the pr: bug fix This PR fixes a bug in a past release. label Mar 23, 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.

Routes including index.html are broken
5 participants