Automatic Static Optimization causes hydration warnings for pages with query string parameters

[MrLeebo]

Simple Repro: https://codesandbox.io/s/misty-lake-dwvg7

What is the problem?

If a page invokes the useRouterQuery() hook, it will generate a hydration warning when you visit the page. This is because Automatic Static Optimization is performed on all pages in Blitz by default, unless the page performs Server Side Rendering.

Say we have the query string ?name=John+Doe and we render <h1>{name}</h1> in some JSX.

- Warning: Text content did not match. Server: "" Client: "John Doe"

Most of the time, something like this is not a big deal because it's just a warning and React already makes a correction. But these mismatches, if left untreated, can introduce more dangerous bugs because, though React attempts to correct mismatches, React may not always make the right call if a query param is part of a conditional expression that renders lots of "similar but not identical" DOM elements between the server and client versions. A mismatch that isn't fixed correctly can lead to a DOM Exception at reconciliation time, and at that point your React app is basically dead in the water until the user manually reloads the page.

Is it possible for Blitz to detect that the page has useRouterQuery() imported and automatically switch to server-side rendering? Seems like that would eliminate this entire category of potential bugs.

Workarounds

For simple text content mismatches, you can add the suppressHydrationWarning prop:

<h1 suppressHydrationWarning>Thank you, {name}!</h1>

And if your query param is part of some page logic or a more complex render expression, you can add a getServerSideProps function to disable the static optimization entirely:

export async function getServerSideProps() {
  return { props: {} };
}

Paste all your error logs here:

- Warning: Text content did not match. Server: "" Client: "John Doe"

Paste all relevant code snippets here:

import { Link, BlitzPage, useRouterQuery } from "blitz";
import Layout from "app/core/layouts/Layout";

const ThankYou: BlitzPage = () => {
  const { name } = useRouterQuery();

  return (
    <div className="container">
      <main>
        <h1>Thank you, {name}!</h1>
        <p>Your order will arrive shortly.</p>
        <Link href="/">Go back</Link>
      </main>
    </div>
  );
};

ThankYou.getLayout = (page) => <Layout title="Home">{page}</Layout>;

export default ThankYou;

What are detailed steps to reproduce this?

1. Add useRouterQuery() to a page
2. Render a param in JSX
3. Visit the page with the param in the query string

Run blitz -v and paste the output here:

From the Blitz.js CodeSandbox Template:

Linux 5.4 | linux-x64 | Node: v14.16.1

blitz: 0.30.0-canary.14 (local)

  Package manager: yarn
  System:
    OS: Linux 5.4 Debian GNU/Linux 10 (buster) 10 (buster)
    CPU: (16) x64 Intel(R) Core(TM) i9-9900K CPU @ 3.60GHz
    Memory: 10.02 GB / 62.73 GB
    Shell: 5.0.3 - /bin/bash
  Binaries:
    Node: 14.16.1 - ~/.nvm/versions/node/v14.16.1/bin/node
    Yarn: 1.22.10 - ~/.nvm/versions/node/v14.16.1/bin/yarn
    npm: 6.14.12 - ~/.nvm/versions/node/v14.16.1/bin/npm
    Watchman: Not Found
  npmPackages:
    @prisma/client: ~2.16 => 2.16.1
    blitz: 0.30.0-canary.14 => 0.30.0-canary.14
    prisma: ~2.16 => 2.16.1
    react: 0.0.0-experimental-3310209d0 => 0.0.0-experimental-3310209d0
    react-dom: 0.0.0-experimental-3310209d0 => 0.0.0-experimental-3310209d0
    typescript: 4.1.5 => 4.1.5

Please include below any other applicable logs and screenshots that show your problem:

[benbender]

Imho the described behaviour isn't a bug, but intended and expected.

Nextjs explicitly states "It [query] will be an empty object during prerendering if the page doesn't have data fetching requirements." (See https://nextjs.org/docs/api-reference/next/router).

If there is a bug, it's that name is an empty string on serverside and not undefined as router.query.name of next/router is.

Which opens the question why the useRouterQuery()-hook exists in the first place? To me it doesn't seem to add any benefit and introduces a bug (empty string on the server instead of undefined) and strange edge-cases (ignores named-params in /foo/[name] while router.query returns it). Or am I missing a point here?

edit: I totally did miss the point as the "edge-case" is the intended usage scenario (see blitz-js/legacy-framework#433) as it allows to parse something like /foo/23?id=42 for the route /foo/[id]. Observation: If you call ?name=jane on the route /foo/[bar], React doesn't complain. Not sure what the next-folk did to suppress it, but it seems to be handled in there codebase.

Coming from a nextjs-background it seems broken to me and I personally don't think this edge-case should be supported at all.

[MrLeebo]

I get what you're saying, but Blitz.js isn't Next.js and the goals of the projects aren't necessarily the same. Blitz is, at least partially, defined by what it builds on top of the foundation laid by Next. In particular, the promises of No-API and eliminating the friction between the client-side and server-side portions of your application.

Those promises are broken if something as pedestrian as a query string parameter can shatter the illusion that Blitz is smart enough to determine whether your code needs to run client-side or server-side. But I don't see any reason why it should stay that way. At compile-time, Blitz can detect if a page is importing useRouterQuery() and automatically switch over to server-side rendering for that page. Why should the rule be "Blitz always performs static optimization unless you manually insert server-side rendering," when it could be "Blitz detects the most appropriate rendering strategy for your page" ?

[benbender]

To be honest, I don't think thats a good idea. It would introduce "magic" instead of "convention over configuration". One would also need to parse the request-object somewhere to extract the querystring. If you already have getServerSideProps() defined, it would need to wrap or patch that method etc etc...

Besides implementation-problems, I have to say that I don't see the benefit of the (arbitrary & synthetical) distinction between "path-based" & "url-based" query-params. So why all the hassle and the additional code? Just for hiding the fact, that internally "path-based" query-params are already simple query-params inside next/router?

[flybayer]

I agree with @benbender that we shouldn't automatically opt into SSR. That's a big change to I feel devs should make themselves.

As for the hydration warning, the proper fix is to initialize query result as undefined on first render.

edit: I totally did miss the point as the "edge-case" is the intended usage scenario (see blitz-js/legacy-framework#433) as it allows to parse something like /foo/23?id=42 for the route /foo/[id]. Observation: If you call ?name=jane on the route /foo/[bar], React doesn't complain. Not sure what the next-folk did to suppress it, but it seems to be handled in there codebase.

Coming from a nextjs-background it seems broken to me and I personally don't think this edge-case should be supported at all.

This is a fair point. Personally I don't have a strong feeling either way. Feel free to post an RFC in our GH discussions to revert to same behavior as Next.js to see if others agree.

[flybayer]

@MrLeebo we still have the hydration warning issue, right?

[MrLeebo]

Yes. It's always going to be a facet of automatic static optimization. I can see why nextjs couldn't do it, but blitz can assume users will be running a web server (can't have queries and mutations otherwise) so automatically switching doesn't seem as drastic a move as it would be for nextjs

[flybayer]

Unless I'm missing something, having a web server or not has zero affect on React hydration on the client.

We should never cause potential hydration issues as it could cause unexpected bugs. So we need to fix this.

[flybayer]

We removed useRouterQuery