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): fix duplicate app render when url accessed through .html extension #10059

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

slorber
Copy link
Collaborator

@slorber slorber commented Apr 19, 2024

Motivation

When a Docusaurus page is accessed through its non-canonical variant ending with .html, we get a weird duplicate render

It looks like React is not able to hydrate and fully recreate an app alongside the existing static markup, leading to duplicated HTML markup

This can be seen in production: https://docusaurus.io/tests.html

CleanShot 2024-04-19 at 10 10 10

Note: technically it's not really a bug because URLs should be accessed through their canonical URLs, that work fine. Users should configure their host properly so that this is always the URL used: https://docusaurus.io/tests

However, not all hosts have access to a "pretty URL" setting that do the redirect to the canonical URL (and apparently Netlify's pretty URL setting does not even work for us...)

I tried investigating the problem and it's very unclear what's causing this issue. It might be React Router v5, or our integration of it through the <PendingNavigation/> component. It looks like a core problem, not a theme-classic nor layout issue because I can reproduce with an empty theme layout too. We have a normalizeLocation function in core (removing the HTML suffix before matching with React Router) but it does not look to be the problem, and using an hardcoded location lead to the same result.

It looks like just adding a wrapper div around the <App> fixes the problem (cf this PR). I suspect this is a bug in React that fails to hydrate when the <App> renders a fragment of multiple elements. I don't mind adding this extra wapper div, it probably wouldn't harm, but let's delay this to v4 considering it might break some user selectors (breaking change? 🤔 ). Also, it's possible that simply upgrading to React Router v6 and React 19 fixes the problem 🤷‍♂️ . But that does not look like a bad idea to add this wrapper for v4 anyway.

Note: to reproduce locally with docusaurus serve, it requires explicitly disabling clean URLs in serve.ts:

    serveHandler(req, res, {
      cleanUrls: false,
      // ... rest
    });

Test Plan

Netlify preview should fine for:

@slorber slorber requested a review from Josh-Cena as a code owner April 19, 2024 08:16
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Apr 19, 2024
@slorber slorber added the pr: bug fix This PR fixes a bug in a past release. label Apr 19, 2024
@slorber slorber marked this pull request as draft April 19, 2024 08:16
@slorber slorber added this to the 4.0 milestone Apr 19, 2024
Copy link

netlify bot commented Apr 19, 2024

[V2]

Name Link
🔨 Latest commit 0ba9812
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/66222955cf1908000896d9fd
😎 Deploy Preview https://deploy-preview-10059--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.

Copy link

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 72 🟢 98 🟢 96 🟢 100 🟠 88 Report
/docs/installation 🟠 63 🟢 96 🟢 100 🟢 100 🟠 88 Report
/docs/category/getting-started 🟠 75 🟢 100 🟢 100 🟢 90 🟠 88 Report
/blog 🟠 69 🟢 100 🟢 100 🟢 90 🟠 88 Report
/blog/preparing-your-site-for-docusaurus-v3 🟠 62 🟢 96 🟢 100 🟢 100 🟠 88 Report
/blog/tags/release 🟠 68 🟢 100 🟢 100 🟠 80 🟠 88 Report
/blog/tags 🟠 75 🟢 100 🟢 100 🟢 90 🟠 88 Report

@slorber slorber marked this pull request as ready for review April 19, 2024 08:22
@slorber slorber added the pr: breaking change Existing sites may not build successfully in the new version. Description contains more details. label Apr 19, 2024
Copy link

Size Change: +143 B (0%)

Total Size: 1.59 MB

Filename Size Change
website/build/assets/css/styles.********.css 113 kB +60 B (0%)
website/build/assets/js/main.********.js 810 kB +50 B (0%)
website/build/index.html 38 kB +33 B (0%)
ℹ️ View Unchanged
Filename Size
website/.docusaurus/codeTranslations.json 2 B
website/.docusaurus/docusaurus.config.mjs 26.7 kB
website/.docusaurus/globalData.json 91.2 kB
website/.docusaurus/i18n.json 930 B
website/.docusaurus/registry.js 247 kB
website/.docusaurus/routes.js 156 kB
website/.docusaurus/routesChunkNames.json 109 kB
website/.docusaurus/site-metadata.json 2.17 kB

compressed-size-action

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: breaking change Existing sites may not build successfully in the new version. Description contains more details. pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
2 participants