Authorized queries run and crash in server rendering

tmcw commented 2 years ago

What is the problem?

Routes that use getServerProps are rendered with SSR. If they use useQuery, the value of the query result, if you have a resolver.authorize call, is undefined. As a result, any page that relies on a useQuery value that is non-nullable and runs on the server will cause that route to crash in production.

Any of the following would be better:

If useQuery's result is nullable even if the query it's referring to is non-nullable, then its return type should be nullable. Right now, TypeScript allows you to rely on non-nullable query results that become nullable.
If useQuery produced the correct result by actually running the authenticated query.
If components with useQuery did not run.

Paste all your error logs here:

This should not happen TypeError: Cannot read property 'email' of undefined
    at UserInfo (/Users/tmcw/p/sentry-example/.next/server/pages/index.js:137:29)
    at tb (/Users/tmcw/p/sentry-example/node_modules/react-dom/cjs/react-dom-server-legacy.node.production.min.js:55:138)
    at wb (/Users/tmcw/p/sentry-example/node_modules/react-dom/cjs/react-dom-server-legacy.node.production.min.js:57:254)
    at W (/Users/tmcw/p/sentry-example/node_modules/react-dom/cjs/react-dom-server-legacy.node.production.min.js:62:89)
    at X (/Users/tmcw/p/sentry-example/node_modules/react-dom/cjs/react-dom-server-legacy.node.production.min.js:64:97)
    at wb (/Users/tmcw/p/sentry-example/node_modules/react-dom/cjs/react-dom-server-legacy.node.production.min.js:59:313)
    at W (/Users/tmcw/p/sentry-example/node_modules/react-dom/cjs/react-dom-server-legacy.node.production.min.js:62:89)
    at X (/Users/tmcw/p/sentry-example/node_modules/react-dom/cjs/react-dom-server-legacy.node.production.min.js:64:97)
    at wb (/Users/tmcw/p/sentry-example/node_modules/react-dom/cjs/react-dom-server-legacy.node.production.min.js:58:27)
    at W (/Users/tmcw/p/sentry-example/node_modules/react-dom/cjs/react-dom-server-legacy.node.production.min.js:62:89)

Paste all relevant code snippets here:

Example query resolver:

import { resolver, Ctx } from "blitz"
import db from "db"

export default resolver.pipe(
  resolver.authorize(),
  async function getCurrentUser(_ = null, { session }) {
    const user = await db.user.findFirst({
      where: { id: session.userId },
      select: { id: true, name: true, email: true, role: true },
      rejectOnNotFound: true,
    })

    return user
  }
)

Example from a slightly-modified new Blitz app that uses a query:

const UserInfo = () => {
  const currentUser = useCurrentUser()
  try {
    console.log(currentUser.email)
  } catch (e) {
    console.log("This should not happen", e)
  }
  return null;
}

What are detailed steps to reproduce this?

Have an app that uses getServerSideProps and an authenticated, non-nullquery

Run `blitz -v` and paste the output here:

% blitz -v
macOS Monterey | darwin-x64 | Node: v16.7.0

blitz: 0.42.4 (global)
blitz: 0.42.4 (local)

  Package manager: yarn
  System:
    OS: macOS 12.0.1
    CPU: (16) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 2.93 GB / 16.00 GB
    Shell: 5.8 - /usr/local/bin/zsh
  Binaries:
    Node: 16.7.0 - ~/n/bin/node
    Yarn: 1.22.4 - ~/.yarn/bin/yarn
    npm: 6.14.15 - /usr/local/bin/npm
    Watchman: Not Found
  npmPackages:
    @prisma/client: 3.4.2 => 3.4.2
    blitz: 0.42.4 => 0.42.4
    prisma: 3.4.2 => 3.4.2
    react: 18.0.0-alpha-5ca4b0433-20211020 => 18.0.0-alpha-5ca4b0433-20211020
    react-dom: 18.0.0-alpha-5ca4b0433-20211020 => 18.0.0-alpha-5ca4b0433-20211020
    typescript: ~4.4 => 4.4.4

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

No response

flybayer commented 2 years ago

@tmcw unfortunately @beerose and I are unable to reproduce the issue in a new blitz app. Maybe there is a cache issue from your blitz upgrade? Try deleting node_modules, .blitz, and .next.

If that doesn't work, can you provide a full repo that we can clone and test? We'd love to get this fixed for you.

tmcw commented 2 years ago

Sorry, working on it. This bug is unbelievably hard to diagnose. What I have so far is that if I manually set __BLITZ_SUSPENSE_ENABLED=true in my environment, I can get pages to successfully render.

But if I don't, the behavior of SSR simply always does what it shouldn't: it crashes, every time, even after wiping node_modules, .blitz, and .next. I thought it might be that I was using Suspense in my layout, but that wasn't it.

tmcw commented 2 years ago

Okay! I've finally distilled a repro!

https://github.com/tmcw/bug-distill

If you run

yarn
yarn build
yarn start

And request the first page, you should get the error.

beerose commented 2 years ago

Thanks for the repo! I can confirm that something weird happens and that the behaviour was different with Blitz 0.38. We'll investigate and get back to you as soon as possible

@flybayer I think the issue (or part of it) is somewhere here: https://github.com/blitz-js/blitz/blob/canary/nextjs/packages/next/data-client/react-query.tsx#L79, as process.env.__BLITZ_SUSPENSE_ENABLED is undefined in that place. That's also why running it with __BLITZ_SUSPENSE_ENABLED=true solves the issue. That also makes sense timing-wise as those files were moved/edited 3 months ago. So we also have to set it somewhere for blitz start?

flybayer commented 2 years ago

@beerose hmm, so BLITZ_SUSPENSE_ENABLED isn't working at all for blitz start or only for blitz start runtime? Ah yeah so maybe it's working at build time where it gets string replaced by webpack, but for gSSP at runtime, it uses env from Node. So yes, probably are not setting BLITZ_SUSPENSE_ENABLED at runtime properly.

beerose commented 2 years ago

So in order to fix it, I first tried to create a minimal repro from a new Blitz app. I used @tmcw's code, but it was working fine, so I compared deps, and turns out it fails only with some experimental react versions (I tried a few). New Blitz apps have 18 alpha so that works, and that's why we weren't able to reproduce it before.

With experimental version and blitz start runtime __BLITZ_SUSPENSE_ENABLED is false in all the places, but I'm not sure what is causing it.

The fix currently is to update React version to an alpha one (or another working version).

beerose commented 2 years ago

I'm going to close this issue, as it doesn't seem to be Blitz specific, but rather something was wrong with a few React versions. Please reopen if this happens again.

tmcw commented 2 years ago

Got it - thanks for digging in to this!

blitz-js / legacy-framework