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

next/dynamic SSR is broken when not using a custom Document, or using a Document with getInitialProps #45151

Closed
1 task done
sebpettersson opened this issue Jan 22, 2023 · 3 comments · Fixed by #45160
Closed
1 task done

next/dynamic SSR is broken when not using a custom Document, or using a Document with getInitialProps #45151

sebpettersson opened this issue Jan 22, 2023 · 3 comments · Fixed by #45160
Assignees
@huozhi
Labels
Lazy Loading Related to Next.js Lazy Loading (e.g., `next/dynamic` or `React.lazy`). linear: next Confirmed issue that is tracked by the Next.js team.

Comments

@sebpettersson
Copy link

Verify canary release

  • I verified that the issue exists in the latest Next.js canary release

Provide environment information

Operating System:
  Platform: darwin
  Arch: arm64
  Version: Darwin Kernel Version 22.2.0: Fri Nov 11 02:03:51 PST 2022; root:xnu-8792.61.2~4/RELEASE_ARM64_T6000
Binaries:
  Node: 18.12.0
  npm: 8.19.2
  Yarn: N/A
  pnpm: 7.12.2
Relevant packages:
  next: 13.1.5-canary.2
  eslint-config-next: N/A
  react: 18.2.0
  react-dom: 18.2.0

Which area(s) of Next.js are affected? (leave empty if unsure)

Dynamic imports (next/dynamic)

Link to the code that reproduces this issue

https://github.com/sebpettersson/next-dynamic-bug

To Reproduce

Import a component with next/dynamic, build the project and then check the rendered markup.

next-dynamic-bug

https://next-dynamic-bug.vercel.app/

Describe the Bug

When using a component imported with next/dynamic, the markup will contain <template> instead of correct SSR content.

NOTE: If JavaScript is enabled it is easy to miss this bug since the <template> will be switched out for the correct content.

Additional findings

  1. This bug was introduced in 13.0.7.
  2. If a custom Document is used the, markup is rendered correctly - provided it is a functional component that doesn't use getInitialProps. If it is a class that extends Document from next/document, or a functional component that uses getInitialProps, the bug is still triggered.
  3. In dev mode, the bug is only triggered the first time the page is loaded. On subsequent page loads the correct markup is output.

Expected Behavior

That server side rendered content contains the correct markup when using components imported with next/dynamic.

Which browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

No response
@sebpettersson sebpettersson added the bug Issue was opened via the bug report template. label Jan 22, 2023
@huozhi huozhi added kind: bug Lazy Loading Related to Next.js Lazy Loading (e.g., `next/dynamic` or `React.lazy`). and removed bug Issue was opened via the bug report template. labels Jan 22, 2023
@huozhi huozhi self-assigned this Jan 22, 2023
@github-actions github-actions bot added the linear: next Confirmed issue that is tracked by the Next.js team. label Jan 22, 2023
@huozhi huozhi mentioned this issue Jan 23, 2023
Merged
3 tasks
@fikrikarim
Copy link
Contributor

Hi @sebpettersson, thanks a lot for making this issue. We've been trying to fix this bug since a week ago.

I tried to reproduce the issue with create-next-app, but apparently, it comes with a custom Document by default. And as you said that when there's a functional custom Document, the markup will be rendered correctly. So we thought that the problem was in our code and not in the Next.js itself.

Looks like @huozhi is already working on the PR to fix this. Thanks for working on it!

@hsheikhali1
Copy link

hsheikhali1 commented Jan 23, 2023 •
edited

I think this also causes a problem when using a dynamic component and attempting to compile down to static.. You don't actually get the compiled HTML.

Here is what I think may also be related as in it might also be a side effect of something similar

#45099

@kodiakhq kodiakhq bot closed this as completed in #45160 Jan 23, 2023
kodiakhq bot pushed a commit that referenced this issue Jan 23, 2023
@huozhi
Fix React.lazy preloading during SSR (#45160)
e6ff749 
## Bug

Previously the `React.lazy` and Loadable preloading are creating different module loading promises with loader. Now we change to wait the loader in `React.lazy` to make sure for SSR case they're preloaded. The case to trigger this bug is: When adding `Document.getInitialProps` (aka. gIP), rendering goes to process the gIP first which would make the lazy elements executed before gIP, then preloading will happen after lazy which leads to Suspense resolves too fast without content.

Fixes #45151

- [x] Related issues linked using `fixes #number`
- [x] Integration tests added
- [ ] Errors have a helpful link attached, see [`contributing.md`](https://github.com/vercel/next.js/blob/canary/contributing.md)
@sebpettersson sebpettersson mentioned this issue Feb 1, 2023
Closed
1 task
@huozhi huozhi mentioned this issue Feb 4, 2023
Merged
3 tasks
kodiakhq bot pushed a commit that referenced this issue Feb 4, 2023
@huozhi
Separate next/dynamic implementation for app and pages (#45565)
a2dc530 
## Issue

To address the problem that we introduced in 13.0.7 (#42589) where we thought we could use same implementation `next/dynamic` for both `pages/` and `app/` directory. But it turns out it leads to many problems, such as:

* SSR preloading could miss the content, especially with nested dynamic calls
  * Closes #45213
* Introducing suspense boundary into `next/dynamic` with extra wrapped `<Suspense>` outside will lead to content is not resolevd during SSR
  * Related #45151
  * Closes #45099
* Unexpected hydration errors for suspense boundaries. Though react removed this error but the 18.3 is not out yet.
  * Closes #44083
  * Closes #45246
 
## Solution

Separate the dynamic implementation for `app/` dir and `pages/`. 

For `app/` dir we can encourage users to: 
  * Directly use `React.lazy` + `Suspense` for SSR'd content, and `next/dynamic` 
  * For non SSR components since it requires some internal integeration with next.js.

For `pages/` dir we still keep the original implementation

If you want to use `<Suspense>` with dynamic `fallback` value, use `React.lazy` + `Suspense` directly instead of picking up `next/dynamic` 
  * Closes #45116

This will solve various issue before react 18.3 is out and let users still progressively upgrade to new versions of next.js.

## Bug Fix

- [x] Related issues linked using `fixes #number`
- [x] Integration tests added
- [ ] Errors have a helpful link attached, see [`contributing.md`](https://github.com/vercel/next.js/blob/canary/contributing.md)
@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 Feb 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@huozhi huozhi

Labels
Lazy Loading Related to Next.js Lazy Loading (e.g., `next/dynamic` or `React.lazy`). linear: next Confirmed issue that is tracked by the Next.js team.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix React.lazy preloading during SSR huozhi/next.js
4 participants
@sebpettersson @huozhi @fikrikarim @hsheikhali1