fix(core): prevent 404 when accessing /page.html #7184

Josh-Cena · 2022-04-17T01:34:14Z

Motivation

Because we use trailingSlash: false in production, https://docusaurus.io/docs/installation.html will send the correct HTML file, but the router doesn't understand it because it only normalizes paths with /index.html suffix. I think we can extend this to .html extensions in general.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Interestingly yarn serve seems to be happy with /docs/installation.html. I don't know if it has anything to do with the cleanUrls or related settings. Anyways, I simply forced trailingSlash: false in the preview. https://deploy-preview-7184--docusaurus-2.netlify.app/docs/installation.html is now accessible.

netlify · 2022-04-17T01:42:57Z

✅ [V2]

Name	Link
🔨 Latest commit	`2b82e53`
🔍 Latest deploy log	https://app.netlify.com/sites/docusaurus-2/deploys/6262b12eb5e018000867ce11
😎 Deploy Preview	https://deploy-preview-7184--docusaurus-2.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

github-actions · 2022-04-17T01:43:14Z

⚡️ Lighthouse report for the changes in this PR:

Category	Score
🟠 Performance	70
🟢 Accessibility	100
🟢 Best practices	92
🟢 SEO	100
🟢 PWA	90

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

github-actions · 2022-04-17T01:44:38Z

Size Change: +356 B (0%)

Total Size: 802 kB

Filename	Size	Change
`website/build/assets/css/styles.********.css`	107 kB	+194 B (0%)
`website/build/assets/js/main.********.js`	606 kB	+152 B (0%)

ℹ️ View Unchanged

Filename	Size	Change
`website/.docusaurus/globalData.json`	50.1 kB	+10 B (0%)
`website/build/index.html`	38.8 kB	0 B

_{compressed-size-action}

slorber

2 important things to consider:

Many legacy doc sites moving to Docusaurus want to preserve older canonical urls for SEO. We have users that explicitly want to use .html in router paths (I even recommended some users to use slug: /xyz.html for site migration purposes) => we should dogfood this and ensure it keeps working, not stripping the extension if the route actually exists.
This page renders better than before but produces FOUC after hydration: https://deploy-preview-7184--docusaurus-2.netlify.app/docs/installation.html , unlike https://deploy-preview-7184--docusaurus-2.netlify.app/docs/installation => that seems unexpected to me, but we'd rather fix it

Josh-Cena · 2022-04-20T23:46:23Z

Many legacy doc sites moving to Docusaurus want to preserve older canonical urls for SEO.

That's the entire purpose of this PR. Before, the .html extension would make the page not render, because Netlify serves the HTML file without a redirect, but the router doesn't understand this location.

This page renders better than before but produces FOUC after hydration

Do you have a repro? It looks the same to me even under throttling.

Simek · 2022-04-21T09:08:10Z

Refs:

pages under docs/ shows flash of content then shows 404 react-native-website#3055

Josh-Cena · 2022-04-21T09:17:42Z

I even recommended some users to use slug: /xyz.html for site migration purposes

TBF, that looks like a horrible pattern. You shouldn't encode the extension in the location. We should have made this fix long ago, then, if people have to commit to such horrible solutions.

slorber · 2022-04-21T09:28:39Z

I even recommended some users to use slug: /xyz.html for site migration purposes

TBF, that looks like a horrible pattern. You shouldn't encode the extension in the location. We should have made this fix long ago, then, if people have to commit to such horrible solutions.

There is no strict rule here.

Some users really want the links to contain the extension, and the canonical URL to contain it too, for SEO and migration reasons.

If you have an existing doc site and all the links contain the extension, and all the canonical URLs known by google contain the extension, then when upgrading you want to minimize the SEO impact and migrate to Docusaurus while keeping this extension in site links and canonical URLs.

Allowing /xyz to be served from /xyz.html is nice but it's not what users want to ensure a proper SEO migration.

That's why I'd like this PR to contain a dogfood doc with slug: "xyz.html", to ensure it's still possible to force the canonical URL and linking of the page to still contain the extension

Josh-Cena · 2022-04-21T09:30:25Z

Okay, I see

slorber · 2022-04-21T09:37:45Z

This page renders better than before but produces FOUC after hydration

Do you have a repro? It looks the same to me even under throttling.

This link is good enough to reproduce: https://deploy-preview-7184--docusaurus-2.netlify.app/docs/installation.html

Just hit refresh, eventually throttle your CPU

video: https://cln.sh/RNi85L

This doesn't happen on https://deploy-preview-7184--docusaurus-2.netlify.app/docs/installation

Josh-Cena · 2022-04-21T09:44:28Z

Okay, I see what you mean. PendingNavigation is not capturing the route correctly and letting the component update, leading to react-loadable's loading view being displayed. Might be because the location is changed twice. Going to take a look later; I already messed up in 49a9fe2 😁

Josh-Cena · 2022-04-22T11:06:08Z

@slorber I've tested. Using slug: dummy.html is working as expected: https://deploy-preview-7184--docusaurus-2.netlify.app/tests/docs/dummy.html You can see that the canonical URL still have the .html extension, and so do the links linking to it. normalizeLocation also wouldn't do anything in this case.

Do you think I need to fix the flash of loading screen in this PR, or should we figure that out later, considering it's much more minor compared to the 404 page before?

slorber · 2022-04-22T12:20:38Z

If it's easy to fix in this PR let's do this now, otherwise we can merge

Similarly, does it make sense to also ensure this URL works? https://deploy-preview-7184--docusaurus-2.netlify.app/tests/docs/dummy => should be quite easy and similar to current code?

Does all this work with trailing slash: true?
(not a big if it's not the case)

slorber · 2022-04-22T12:21:17Z

packages/docusaurus/src/client/normalizeLocation.ts

@@ -18,8 +20,16 @@ export default function normalizeLocation<T extends Location>(location: T): T {
    };
  }

+  // If the location was registered with an `.html` extension, we don't strip it
+  // away, or it will render to a 404 page.
+  const matchedRoutes = matchRoutes(routes, location.pathname);


also handle matchRoutes(routes, location.pathname + ".html"); ?

I wouldn't do that for now, because it makes less sense.

/docs/installation.html is a physical file that exists and can be served, so using this as a location makes sense. If the route is /docs/installation, we should try to normalize /docs/installation.html to that.

However, if both the route and the physical file is /docs/installation.html, then /docs/installation doesn't really make sense—it's nowhere to be found. Moreover, I'm worried that this would lead to more edge cases, especially around trailing slash and index.html.

website/docusaurus.config.js

Josh-Cena · 2022-04-22T12:25:19Z

Does all this work with trailing slash: true?

We can test this before merging, when I revert the trailingSlash config change

Josh-Cena · 2022-04-22T13:45:15Z

Everything works as expected @slorber

The PendingNavigation looks slightly non-trivial. I'd need to investigate it more.

fix(core): normalize /path.html to /path to prevent 404

eac5fe7

Josh-Cena added the pr: bug fix This PR fixes a bug in a past release. label Apr 17, 2022

Josh-Cena requested review from slorber and lex111 as code owners April 17, 2022 01:34

facebook-github-bot added the CLA Signed Signed Facebook CLA label Apr 17, 2022

test no trailing slash

76c2f2c

Josh-Cena changed the title ~~fix(core): normalize /path.html to /path to prevent 404~~ fix(core): normalize /path.html location to /path to prevent 404 Apr 17, 2022

slorber requested changes Apr 20, 2022

View reviewed changes

dogfood?

49a9fe2

Josh-Cena added 7 commits April 22, 2022 17:53

Merge branch 'main' into jc/fix-normalize-location

9157cd5

revert last change

5134e87

does this fix

77513e1

ignore

0cf606a

add test

63447e0

fix

67fa969

properly fix

91e0fdb

fix again

d4c982e

Josh-Cena changed the title ~~fix(core): normalize /path.html location to /path to prevent 404~~ fix(core): prevent 404 when accessing /page.html Apr 22, 2022

Josh-Cena requested a review from slorber April 22, 2022 11:30

slorber approved these changes Apr 22, 2022

View reviewed changes

Josh-Cena added 2 commits April 22, 2022 21:30

revert trailing slash change

12eeedb

optimize: use map instead of object

2b82e53

slorber merged commit c4e92c8 into main Apr 22, 2022

slorber deleted the jc/fix-normalize-location branch April 22, 2022 15:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): prevent 404 when accessing /page.html #7184

fix(core): prevent 404 when accessing /page.html #7184

Josh-Cena commented Apr 17, 2022 •

edited

Loading

netlify bot commented Apr 17, 2022 •

edited

Loading

github-actions bot commented Apr 17, 2022 •

edited

Loading

github-actions bot commented Apr 17, 2022 •

edited

Loading

slorber left a comment

Josh-Cena commented Apr 20, 2022 •

edited

Loading

Simek commented Apr 21, 2022

Josh-Cena commented Apr 21, 2022

slorber commented Apr 21, 2022 •

edited

Loading

Josh-Cena commented Apr 21, 2022

slorber commented Apr 21, 2022

Josh-Cena commented Apr 21, 2022

Josh-Cena commented Apr 22, 2022

slorber commented Apr 22, 2022

slorber Apr 22, 2022

Josh-Cena Apr 22, 2022

Josh-Cena commented Apr 22, 2022

Josh-Cena commented Apr 22, 2022 •

edited

Loading

fix(core): prevent 404 when accessing /page.html #7184

fix(core): prevent 404 when accessing /page.html #7184

Conversation

Josh-Cena commented Apr 17, 2022 • edited Loading

Motivation

Have you read the Contributing Guidelines on pull requests?

Test Plan

netlify bot commented Apr 17, 2022 • edited Loading

✅ [V2]

github-actions bot commented Apr 17, 2022 • edited Loading

github-actions bot commented Apr 17, 2022 • edited Loading

slorber left a comment

Choose a reason for hiding this comment

Josh-Cena commented Apr 20, 2022 • edited Loading

Simek commented Apr 21, 2022

Josh-Cena commented Apr 21, 2022

slorber commented Apr 21, 2022 • edited Loading

Josh-Cena commented Apr 21, 2022

slorber commented Apr 21, 2022

Josh-Cena commented Apr 21, 2022

Josh-Cena commented Apr 22, 2022

slorber commented Apr 22, 2022

slorber Apr 22, 2022

Choose a reason for hiding this comment

Josh-Cena Apr 22, 2022

Choose a reason for hiding this comment

Josh-Cena commented Apr 22, 2022

Josh-Cena commented Apr 22, 2022 • edited Loading

Josh-Cena commented Apr 17, 2022 •

edited

Loading

netlify bot commented Apr 17, 2022 •

edited

Loading

github-actions bot commented Apr 17, 2022 •

edited

Loading

github-actions bot commented Apr 17, 2022 •

edited

Loading

Josh-Cena commented Apr 20, 2022 •

edited

Loading

slorber commented Apr 21, 2022 •

edited

Loading

Josh-Cena commented Apr 22, 2022 •

edited

Loading