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(core, plugin-content-docs): Render NotFoundContent via route if routeBasePath is / #9719

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

thejcannon
Copy link

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

Fixes #9688

The behavior was semi-inspired by option 1 to #9688 (comment)

Test Plan

(Let me know what else needs to happen)

On the repo which reproduces #9688, I made this change (e.g. leveraged my fork) and was successfully able to get a 404 on any /<whatever> page, while still getting the content on the correct docs subroutes.

Test links

Deploy preview: https://deploy-preview-_____--docusaurus-2.netlify.app/

Related issues/PRs

Copy link

github-actions bot commented Jan 8, 2024

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 74 🟢 98 🟢 100 🟢 100 🟠 89 Report
/docs/installation 🟠 66 🟢 98 🟢 100 🟢 100 🟠 89 Report
/docs/category/getting-started 🟠 74 🟢 100 🟢 100 🟢 90 🟠 89 Report
/blog 🟠 70 🟢 100 🟢 100 🟢 90 🟠 89 Report
/blog/preparing-your-site-for-docusaurus-v3 🟠 64 🟢 97 🟢 100 🟢 100 🟠 89 Report
/blog/tags/release 🟠 71 🟢 100 🟢 100 🟠 80 🟠 89 Report
/blog/tags 🟠 76 🟢 100 🟢 100 🟢 90 🟠 89 Report

Copy link

netlify bot commented Jan 8, 2024

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit 4d1fd5e
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/659c684247277e000701d791
😎 Deploy Preview https://deploy-preview-9719--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -43,6 +43,24 @@ export default function ComponentCreator(
},
});
}
if (path === '/*') {
Copy link
Author

Choose a reason for hiding this comment

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

This feels hacky, but I'm just not familiar enough with the ecosystem or Docusaurus code to make that call. So here goes 😄

@@ -42,6 +42,14 @@ export function sortConfig(
return -1;
}

// Wildcard also should get placed last
if (a.path.endsWith('/*')) {
Copy link
Author

Choose a reason for hiding this comment

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

I couldn't decide between a.path == "/*" or .endsWith and decided to go with endsWith since it seemed less magical.

@thejcannon
Copy link
Author

Oh, and if y'all want I can (temporarily) change the website to use / as the routeBasePath as a demonstration 👍

Copy link

netlify bot commented Jan 8, 2024

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit 25d45e8
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/659db9c9300819000875392c
😎 Deploy Preview https://deploy-preview-9719--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@thejcannon thejcannon marked this pull request as draft January 9, 2024 03:11
@thejcannon thejcannon marked this pull request as ready for review January 9, 2024 03:13
@thejcannon thejcannon changed the title Render NotFoundContent via route if routeBasePath is / fix(core, plugin-content-docs): Render NotFoundContent via route if routeBasePath is / Jan 9, 2024
@thejcannon thejcannon force-pushed the jcannon/fix404 branch 2 times, most recently from c440861 to 25d45e8 Compare January 9, 2024 21:25
if (param.options.routeBasePath === '/') {
subRoutes.push({
path: '/*',
component: '@docusaurus/ComponentCreator',
Copy link
Author

Choose a reason for hiding this comment

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

What's the benefit of using ComponentCreator here instead of NotFound/Content? Theoretically, couldn't the ComponentCreator("*") at the end of the routes just use @theme/NotFound?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

404 page not rendered when routeBasePath is "/", and no version uses that path
2 participants