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.

Sign up for GitHub

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

Jump to bottom

React.lazy() content HTML does not get included in SSG output #33879

Closed
gaearon opened this issue Feb 1, 2022 · 11 comments · Fixed by #34106
Closed

React.lazy() content HTML does not get included in SSG output #33879

gaearon opened this issue Feb 1, 2022 · 11 comments · Fixed by #34106
Labels
bug Issue was opened via the bug report template.

Comments

@gaearon
Copy link
Contributor

gaearon commented Feb 1, 2022

Run next info (available from version 12.0.8 and up)

Operating System:
  Platform: darwin
  Arch: x64
  Version: Darwin Kernel Version 21.2.0: Sun Nov 28 20:28:54 PST 2021; root:xnu-8019.61.5~1/RELEASE_X86_64
Binaries:
  Node: 16.13.0
  npm: 8.1.0
  Yarn: 1.22.0
  pnpm: N/A
Relevant packages:
  next: 12.0.10
  react: 0.0.0-experimental-fa816be7f-20220128
  react-dom: 0.0.0-experimental-fa816be7f-20220128

What version of Next.js are you using?

12.0.10

What version of Node.js are you using?

16.13.0

What browser are you using?

Chrome

What operating system are you using?

macOS

How are you deploying your application?

next start

Describe the Bug

@devknoll asked me to file an issue for this. Basically, I'm observing that with React experimental versions, SSG doesn't wait for lazy() to be resolved and outputs the Suspense fallback HTML. Whereas I would've expected it to include content in SSG.

I'm not sure if this means import() needs to be compiled differently, or something.

A similar issue is that in SSR mode, the first hit will get fallback HTML and the next hits will get content HTML. Because the module is cached now.

Expected Behavior

HTML output of SSG includes the SSR content of the lazy child.

To Reproduce

Create a new clean project.

Add experimental React versions to it.

Page code:

import {lazy, Suspense} from 'react'
import Head from 'next/head'

const Child = lazy(() => import('../components/Child'))

export default function Home() {
  return (
    <div>
      <Head>
        <title>Create Next App</title>
        <meta name="description" content="Generated by create next app" />
        <link rel="icon" href="/favicon.ico" />
      </Head>

      <main>
        <h1>
          Welcome to <a href="https://nextjs.org">Next.js!</a>
        </h1>
        <Suspense fallback={<p style={{ color: 'red' }}>Loading...</p>}>
          <Child />
        </Suspense>
      </main>
    </div>
  )
}

Component code:

// Some heavy lib to simulate loading a large chunk
import {marked} from 'marked'

export default function Child() {
  return <div dangerouslySetInnerHTML={{
    __html: marked(`# hello, world!`)
  }} />
}

Run yarn build and then yarn start.

Observe that the served HTML does not include hello, world! but does include Loading....
@gaearon gaearon added the bug Issue was opened via the bug report template. label Feb 1, 2022
@devknoll
Copy link
Contributor

devknoll commented Feb 1, 2022

QQ: Is setting concurrentFeatures part of the repro too?

@gaearon
Copy link
Contributor Author

gaearon commented Feb 1, 2022 •
edited
Loading

No. (But SSR appears to work without it, and it does include "hello, world" in SSR output with yarn dev after first request.)

@gaearon gaearon mentioned this issue Feb 1, 2022
Merged
@devknoll
Copy link
Contributor

devknoll commented Feb 1, 2022

after the first request

Without concurrentFeatures set, we use the legacy renderToString method, so we won't wait for Suspense. It seems like it's probably suspending the first time (hence the fallback) but not the second time because the module is already loaded.

@gaearon
Copy link
Contributor Author

gaearon commented Feb 1, 2022

Ahhhh that's why it worked at all. I see.

@devknoll
Copy link
Contributor

devknoll commented Feb 1, 2022

Yeah, this is one of several reasons that I think we'd really be in favor of an option to preserve React 17 Suspense SSR semantics unless explicitly opted in

@gaearon
Copy link
Contributor Author

gaearon commented Feb 1, 2022

I see. So basically there's no way to try this now (though we always do SSG) because opting into the flag enables the runtime, etc. But then when that issue is fixed, we can try again.

@sebmarkbage
Copy link
Contributor

Yeah, this is one of several reasons that I think we'd really be in favor of an option to preserve React 17 Suspense SSR semantics unless explicitly opted in

You mean in renderToString? If the way you opt into the new "streaming" mode, even if buffered, is by upgrading to React 18, then you'd get both upgrades so there's no way to get into this intermediate state.

You can still get the problem with data being different but lots of legit use cases don't have those (React.lazy being one) issues. So I think you really need to discover it by seeing the bug in your library.

@sebmarkbage
Copy link
Contributor

The way the client upgrade path works is that the existing API keeps working but warns you to rename to createRoot. So it's deprecated but you have an upgrade path. We could do the same for renderToString but it would be the same API signature so if you do need to keep it, it would be unnecessary. However, doing the exercise would force you to make a choice and hopefully use the streaming one. The end result would be that Next.js should only have the streaming one on though.

@devknoll
Copy link
Contributor

devknoll commented Feb 1, 2022

That makes sense. Let me see why this isn't already the case.

@devknoll
Copy link
Contributor

devknoll commented Feb 2, 2022

Yeah, I think we can change this (I opened #33886). We should probably also make next/dynamic not require concurrentFeatures too.

@devknoll devknoll mentioned this issue Feb 8, 2022
Merged
@kodiakhq kodiakhq bot closed this as completed in #34106 Feb 8, 2022
kodiakhq bot pushed a commit that referenced this issue Feb 8, 2022
@devknoll
Use renderToStream with React 18 (#34106)
df29561 
As per React 18 recommendation, we should use e.g. `renderToReadableStream` whenever we use `createRoot`. This is particularly important for currently supported suspense features like `React.lazy` to work properly during SSR.

However, unless you have opted in to streaming support (via [the `runtime` flag](#34068)), we will wait until `onCompleteAll` before sending it (via the `generateStaticHTML` flag).

---

Fixes #33879
natew pushed a commit to natew/next.js that referenced this issue Feb 16, 2022
@devknoll @natew
Use renderToStream with React 18 (vercel#34106)
a24ef59 
As per React 18 recommendation, we should use e.g. `renderToReadableStream` whenever we use `createRoot`. This is particularly important for currently supported suspense features like `React.lazy` to work properly during SSR.

However, unless you have opted in to streaming support (via [the `runtime` flag](vercel#34068)), we will wait until `onCompleteAll` before sending it (via the `generateStaticHTML` flag).

---

Fixes vercel#33879
@github-actions
Copy link
Contributor

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
bug Issue was opened via the bug report template.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Use renderToStream with React 18 azukaru/next.js
3 participants
@sebmarkbage @gaearon @devknoll