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): refactor routes.ts + add route hash for chunkNames key #3001

Merged
merged 4 commits into from
Jun 30, 2020

Conversation

slorber
Copy link
Collaborator

@slorber slorber commented Jun 26, 2020

Motivation

See #2917

Fix potential weird merge behavior when a parent route and its child has the exact same path.

addRoute({
  path: "/docs", // <===
  exact: false,
  component: DocPage,
  routes: [
    {
      path: "/docs", // <===
      exact: true,
      component: DocItem
    }
  ]
});

To avoid potential conflicts, smallHash(route) is added at the end of the chunknames key.

Also refactors the routes.ts file by splitting a bit the logic and trying to make it more readable than before (inline code generation strings do not help for readability)

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

tests + preview

@slorber slorber requested a review from yangshun as a code owner June 26, 2020 17:34
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jun 26, 2020
@slorber slorber added pr: bug fix This PR fixes a bug in a past release. pr: maintenance This PR does not produce any behavior differences to end users when upgrading. labels Jun 26, 2020
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Jun 26, 2020

Deploy preview for docusaurus-2 ready!

Built with commit 9beb8c9

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

// Important: the layout component should not end with /,
// as it conflicts with the home doc
// Workaround fix for https://github.com/facebook/docusaurus/issues/2917
const docsPath = docsBaseRoute === '/' ? '' : docsBaseRoute;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

feature permits to remove that temporary hack that's not needed anymore

@@ -21,7 +24,8 @@ function ComponentCreator(path: string): ReturnType<typeof Loadable> {
});
}

const chunkNames = routesChunkNames[path];
const chunkNamesKey = `${path}-${hash}`;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I prefered reconstructing the chunkNamesKey with the same logic on client because chunkNamesKey already contains the path so this would look more messy if user inspects .docusaurus folder, and would also take more place

routesPaths.push(routePath);
}

function genRouteChunkNames(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fn extracted, not really modified

: ''
}}`;
return str;
};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

trying to encapsulate all string codegen here

: ''
}
`,
),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

warning I put recently, can be removed now because conflits are unlikely to happen

@slorber
Copy link
Collaborator Author

slorber commented Jun 26, 2020

ready to review

@@ -187,3 +157,44 @@ export default [
routesPaths,
};
}

function genRouteChunkNames(
// TODO instead of passing a mutating the registre, return a registry slice?
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo - Register

@slorber slorber merged commit cf97662 into master Jun 30, 2020
@yangshun yangshun deleted the slorber/duplicate-routes branch June 30, 2020 14:55
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. pr: maintenance This PR does not produce any behavior differences to end users when upgrading.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

routesChunkName.json does not allow sub-route with same path as parent.
4 participants