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: place root route at the end #5645

Merged
merged 3 commits into from
Oct 6, 2021
Merged

fix: place root route at the end #5645

merged 3 commits into from
Oct 6, 2021

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Oct 4, 2021

Motivation

Should fix #3810

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

See snapshot.

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

@lex111 lex111 added the pr: bug fix This PR fixes a bug in a past release. label Oct 4, 2021
@lex111 lex111 requested a review from slorber as a code owner October 4, 2021 14:52
@netlify
Copy link

netlify bot commented Oct 4, 2021

✔️ [V2]

🔨 Explore the source changes: 7e866d0

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/615d8949bdbba200073e46a8

😎 Browse the preview: https://deploy-preview-5645--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Oct 4, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 89
🟢 Accessibility 98
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-5645--docusaurus-2.netlify.app/

@github-actions
Copy link

github-actions bot commented Oct 4, 2021

Size Change: -4 B (0%)

Total Size: 834 kB

ℹ️ View Unchanged
Filename Size Change
website/.docusaurus/globalData.json 38.3 kB 0 B
website/build/assets/css/styles.********.css 93.9 kB 0 B
website/build/assets/js/main.********.js 420 kB 0 B
website/build/blog/2017/12/14/introducing-docusaurus/index.html 67 kB 0 B
website/build/blog/index.html 38.1 kB 0 B
website/build/docs/index.html 44.6 kB -2 B (0%)
website/build/docs/installation/index.html 52.8 kB -2 B (0%)
website/build/index.html 30.8 kB 0 B
website/build/tests/docs/index.html 25.5 kB 0 B
website/build/tests/docs/standalone/index.html 22.9 kB 0 B

compressed-size-action

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Oct 4, 2021
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.

Test snapshot highlights the sort algo is wrong.

I suggest improving test case to ensure "/" with subroute is the very last item after sorting

},
Object {
"component": "",
"path": "/",
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is not in the correct place

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually this is intentional, sorting by root routes first makes it easier to sort by root routes. After all, it doesn't really matter where just root route is, as long as root route with subroutes is placed at the last. But I moved new sorting to the beginning as you asked, probably it is better for consistency as well.

@@ -47,6 +47,11 @@ export function sortConfig(routeConfigs: RouteConfig[]): void {
}
}

// Root route should get placed last.
if (a.path === '/' || b.path === '/') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test is probably good enough, cf snapshot order.

If a !== "/" && b === "/" it should rather return +1

It should work better to do this test at the very beginning of the sort algo instead of the end.

Note if both are "/", we should not return because the route with "/" + subroutes should be the very last

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, done.

@slorber
Copy link
Collaborator

slorber commented Oct 6, 2021

Thanks, that seems good enough to fix the issue above.

modified the test for a better coverage of a edge case


IMHO we still have a problem that we should try to solve: if blog and docs both use / and have subroutes, one will be picked in priority.

We should probably implement our own renderRoutes so that routes with subroutes are rendered only if one of the children match: this would make the system less dependent on route ordering (unless a doc slug conflict with a blog slug...) and also allow multiple instances of the docs plugin to be hosted at /

This logic may not be good enough in our case:
https://github.com/remix-run/react-router/blob/main/packages/react-router-config/modules/renderRoutes.js

@slorber
Copy link
Collaborator

slorber commented Oct 6, 2021

Not urgent, but created an issue: #5651

@slorber slorber merged commit e6e2db7 into main Oct 6, 2021
@slorber slorber deleted the lex111/iss3810 branch October 6, 2021 12:00
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.

In docs only mode, the secondary 'docs' is gone
3 participants