Skip to content

Public site: Get Started buttons html is getting clobbered by main JS #6350

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

Closed
JoshuaKGoldberg opened this issue Jan 18, 2023 · 8 comments · Fixed by #6351
Closed

Public site: Get Started buttons html is getting clobbered by main JS #6350

JoshuaKGoldberg opened this issue Jan 18, 2023 · 8 comments · Fixed by #6351
Labels
accepting prs Go ahead, send a pull request that resolves this issue repo maintenance things to do with maintenance of the repo, and not with code/docs

Comments

@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commented Jan 18, 2023

Suggestion

Continuing #6081: on typescript-eslint.io right now, the Get Started buttons are the wrong size:

Local Production
Screenshot of large font size and padding Getting Started button Screenshot of small font size and padding Getting Started button

Specifically, the server-rendered HTML has the right HTML+CSS, but once the main JS chunk loads in, the button becomes small again. I believe it's from React hydrating with old component JS data.

I poked at things locally (thanks very much again to @AgentEnder for the help!):

  • It happens on local dev, not just Netlify deploys
  • It happens even if we Clear Cache and Deploy Site on Netlify
  • It happens even if we upgrade Nx and Lerna
  • It goes away locally if you rm -rf packages/website/build

...so I believe the conclusion is that there's something wrong with how build/ outputs are not getting cleaned between Docusaurus rebuilds.

@Josh-Cena - do you have any insights here?

@JoshuaKGoldberg JoshuaKGoldberg added triage Waiting for team members to take a look repo maintenance things to do with maintenance of the repo, and not with code/docs labels Jan 18, 2023
@Josh-Cena
Copy link
Member

How can I reproduce it? Do I need to switch between two commits?

@JoshuaKGoldberg
Copy link
Member Author

It happens for me on main. The linked issue has a video

@Josh-Cena
Copy link
Member

Josh-Cena commented Jan 19, 2023

  • It goes away locally if you rm -rf packages/website/build

Does the packages/website/build before come from an older commit? Or do I just need to build and build again?

Never mind, I can reproduce it now...

@Josh-Cena
Copy link
Member

Josh-Cena commented Jan 19, 2023

This is not a cache problem (because I started fresh), but a hydration issue. Here's the HTML before hydration (with JS disabled):

image

Here's what's after:

image

The classNames are applied to the wrong element. This is a quintessential issue of markup mismatch between SSR and first CSR. In this case, the culprit is that the initial HTML is wrong—note how there are two empty <p> tags in the first screenshot! This prompts me to think it's caused by the minifier trying to parse invalid HTML. So I went back and noticed a common pitfall: we are rendering a <div> inside a <p>, which is invalid because <p> only accepts phrasing content. Because the end tag of <p> can be omitted, a conforming HTML parser can interpret the <div> tag as the implicit end of the <p> tag, which is what the minifier is doing, thus causing the issue. A similar issue of invalid HTML was reported here: facebook/docusaurus#8235

You can see for your self with this very simple repro:

const $ = require("react").createElement;
const { minify } = require("html-webpack-plugin/node_modules/html-minifier-terser");

function Home() {
  return $("p", null, $("div", null));
}

const html = require("react-dom/server").renderToString($(Home, null));
console.log("Raw render:");
console.log(html);
console.log("Minified:");
minify(html).then(console.log);
Raw render:
<p><div></div></p>
Minified:
<p></p><div></div><p></p>

The solution is to fix the markup because we should write conformant HTML.

@bradzacher
Copy link
Member

I'm surprised there isn't a lint rule in eslint-plugin-react for this.

I found this 3rd party plugin:
https://github.com/MananTank/eslint-plugin-validate-jsx-nesting

@Josh-Cena
Copy link
Member

This is quite hard to validate, particularly in this case where it's <p>{description}</p> and description is an external variable containing a <div>... It needs some sort of reference tracing.

@bradzacher
Copy link
Member

I'm also pretty sure react does log a warning here when it does the rendering. It contains utils that validate the DOM structure iirc.

@JoshuaKGoldberg JoshuaKGoldberg added accepting prs Go ahead, send a pull request that resolves this issue and removed triage Waiting for team members to take a look labels Jan 19, 2023
@JoshuaKGoldberg JoshuaKGoldberg changed the title Public site: docusaurus build cache isn't getting overridden between deploys Public site: Get Started buttons html is getting clobbered by scripts Jan 19, 2023
@JoshuaKGoldberg JoshuaKGoldberg changed the title Public site: Get Started buttons html is getting clobbered by scripts Public site: Get Started buttons html is getting clobbered by main JS Jan 19, 2023
@JoshuaKGoldberg
Copy link
Member Author

Yeah we should add an end-to-end test to make sure no React warnings are logged. #6354

Thanks!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepting prs Go ahead, send a pull request that resolves this issue repo maintenance things to do with maintenance of the repo, and not with code/docs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants