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

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

Open
7 tasks done
thejcannon opened this issue Jan 2, 2024 · 13 comments · May be fixed by #9719
Open
7 tasks done

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

thejcannon opened this issue Jan 2, 2024 · 13 comments · May be fixed by #9719
Labels
bug An error in the Docusaurus core causing instability or issues with its execution

Comments

@thejcannon
Copy link

thejcannon commented Jan 2, 2024

Have you read the Contributing Guidelines on issues?

Prerequisites

  • I'm using the latest version of Docusaurus.
  • I have tried the npm run clear or yarn clear command.
  • I have tried rm -rf node_modules yarn.lock package-lock.json and re-installing packages.
  • I have tried creating a repro with https://new.docusaurus.io.
  • I have read the console error message carefully (if applicable).

Description

Combining routeBasePath: '/', with having each version at a specific path (the version itself) results in unknown pages rendering "blank" content (instead of the "Page not found" content).

Reproducible demo

https://stackblitz.com/edit/github-py3g6d?file=docusaurus.config.js

Steps to reproduce

From a fresh project:

$ npm run docusaurus docs:version 1.1.0

And add the following to the docs config:

          routeBasePath: '/',
          versions: {
            current: { path: '/current' },
            '1.1.0': { path: '/1.1.0' },
          },

then visit /foo route. No "Page Not Found" content 😢

Expected behavior

Render the "Page not found" content

Actual behavior

Empty page

Your environment

No response

Self-service

  • I'd be willing to fix this bug myself.
@thejcannon thejcannon added bug An error in the Docusaurus core causing instability or issues with its execution status: needs triage This issue has not been triaged by maintainers labels Jan 2, 2024
@slorber
Copy link
Collaborator

slorber commented Jan 3, 2024

Related to of #9688

Not really a duplicate. However the problem/solution is likely to be the same: make react-router-config render top-level 404 if no subroute matches current path

@slorber slorber marked this as a duplicate of #9688 Jan 3, 2024
@slorber slorber closed this as not planned Won't fix, can't repro, duplicate, stale Jan 3, 2024
@slorber slorber added closed: duplicate This issue or pull request already exists in another issue or pull request and removed status: needs triage This issue has not been triaged by maintainers labels Jan 3, 2024
@slorber slorber reopened this Jan 3, 2024
@slorber slorber removed the closed: duplicate This issue or pull request already exists in another issue or pull request label Jan 3, 2024
@thejcannon
Copy link
Author

Related to of #9688

Not really a duplicate. However the problem/solution is likely to be the same: make react-router-config render top-level 404 if no subroute matches current path

That issue number is this issue. Did you mean to link to something else?

@thejcannon
Copy link
Author

Also, any suggestions on a workaround?

@slorber
Copy link
Collaborator

slorber commented Jan 4, 2024

Sorry 🤪 #9665

I think a workaround could be to create a 404.md doc with slug /, but didn't try. If this doesn't work I don't see any other workaround.

@thejcannon
Copy link
Author

However the problem/solution is likely to be the same: make react-router-config render top-level 404 if no subroute matches current path

I'd love to help pitch in if possible! Feel free to thoughtdump how someone would go about doing this 😄

@huonw
Copy link

huonw commented Jan 5, 2024

There's some bits of code that seem to have the intention of showing the <NotFound> 404 component when there's no route match... but apparently not triggering?

@Josh-Cena
Copy link
Collaborator

@huonw You have found out how we show 404 "in general". However, the docs plugin is weirder: it declares a wildcard subpath match (like /{routeBasePath}/*) so that the sidebar never re-renders on navigation. In this case, a "not found" would not be caught by the root wildcard, but by /docs/*.

@huonw
Copy link

huonw commented Jan 5, 2024

Ah, right, okay, so with routeBasePath == / as we configured, the docs plugin catch-all route is /* and presumably is in the route list before that final catch-all, and thus wins.

@Josh-Cena
Copy link
Collaborator

Yes, exactly :D

@slorber
Copy link
Collaborator

slorber commented Jan 5, 2024

Curious, why is the not found working on our own website? 🤔
https://docusaurus.io/docs/notFound
Why does it only happen when routeBasePath is /, I found this surprising.


There are a few ways we could handle it.

  1. Automatically add a * not found route to all subroutes, similar to what is done at the root here:
CleanShot 2024-01-05 at 12 18 44@2x
  1. Fix React-Router-Config route rendering logic

Currently we render routes with a package:

export {renderRoutes as default} from 'react-router-config';

Problem: if a parent route match the path, but no child route match the path, then it does not "escape" the parent route and try to enter another parent route that also matches, nor does it try to render the default * 404 route.

This is IMHO what we should fix.

This would also enable us to have 2 plugins use routeBasePath as / (even if I wouldn't recommend this due to potential conflicts between plugins, it should technically be possible)

  1. Have a way for plugins to declare their custom 404 page

This can be interesting because some users might have a dedicated 404 @theme/DocPageNotFound to say that the doc does not exist, with a link to the root of the version.

That's probably overkill (and requires CDN config to route /docs/* 404 to /docs/notFound.html) but I already saw someone requesting that kind of thing.

Not a priority IMHO


I would go with solution 2, but that's probably not easy.
We'd need to fix that code: https://github.com/remix-run/react-router/blob/v5/packages/react-router-config/modules/renderRoutes.js

Note this code doesn't even exist anymore in React-Router v6 (and we'll probably want to upgrade later so need to make sure our solution will be compatible)

@thejcannon
Copy link
Author

Note this code doesn't even exist anymore in React-Router v6

I wonder if we're ought to try and see if this bug exists after upgrading. You might be able to feed two birds with one scone.

And if not, we likely ought to file an issue so it's made known to the maintainer(s)

@thejcannon
Copy link
Author

Okie dokie, I know it wasn't your first choice, but decided to take a stab at the "easy" solution here of 1.

#9719

I'm relatively noviced when it comes to JS (and furthermore TS), so feel free to tear me a new one with suggestions/questions/etc... I try and learn through osmosis.

@thejcannon
Copy link
Author

@slorber there is a workaround, although it's not for the faint of heart 😆

pantsbuild/pantsbuild.org#84

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An error in the Docusaurus core causing instability or issues with its execution
Projects
None yet
5 participants
@slorber @huonw @thejcannon @Josh-Cena and others