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.

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

Nested next/dynamic is not preloaded during SSR #45213

Closed
1 task done
fikrikarim opened this issue Jan 24, 2023 · 4 comments · Fixed by #45565
Closed
1 task done

Nested next/dynamic is not preloaded during SSR #45213

fikrikarim opened this issue Jan 24, 2023 · 4 comments · Fixed by #45565
Labels
bug Issue was opened via the bug report template. Lazy Loading Related to Next.js Lazy Loading (e.g., `next/dynamic` or `React.lazy`).

Comments

@fikrikarim
Copy link
Contributor

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 21.4.0: Fri Mar 18 00:46:32 PDT 2022; root:xnu-8020.101.4~15/RELEASE_ARM64_T6000
Binaries:
  Node: 16.18.1
  npm: 8.19.2
  Yarn: 1.22.19
  pnpm: N/A
Relevant packages:
  next: 13.1.6-canary.0
  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)

No response

Link to the code that reproduces this issue

https://github.com/fikrikarim/nested-next-dynamic-bug

To Reproduce

Create a nested next/dynamic component: import a component with next/dynamic, and inside that component, import another component with next/dynamic.

Run next build and next start. Check the generated HTML.

image

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

Describe the Bug

The recent PR #45160 fix React.lazy preloading during SSR. It's working, but it doesn't preload a nested next/dynamic.

Expected Behavior

The generated HTML should contain the correct content by preloading all next/dynamic, even the nested ones.

Next.js v13.0.6 has the expected behavior:

image

https://nested-next-dynamic-dbn5z8i2d-fikrikarim.vercel.app/

Which browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

No response

@fikrikarim fikrikarim added the bug Issue was opened via the bug report template. label Jan 24, 2023
@hsheikhali1
Copy link

Can confirm this is still a bug. We're still experiencing issues where static builds dont contain the compiled CSS rather a inline JS script that targets elements.. which then get blocked by CSP rules.

@sebpettersson
Copy link

I think this is a fairly major issue still, since @huozhi's #45160 only partially fixed #45151.

It essentially keeps us from upgrading past 13.0.6, at least without doing changes to our code splitting strategy.

I think this bug might also have SEO implications, and even worse, it might break sites that have CSP that doesn't allow unsafe-inline for script-src.

@hsheikhali1
Copy link

I think this is a fairly major issue still, since @huozhi's #45160 only partially fixed #45151.

It essentially keeps us from upgrading past 13.0.6, at least without doing changes to our code splitting strategy.

I think this bug might also have SEO implications, and even worse, it might break sites that have CSP that doesn't allow unsafe-inline for script-src.

This does break our site since we have very strict CSP rules. We already had to work around the fact that NextJS injects a inline style to image tags.. <img style="color: transparent" ... /> all thought that one isn't as bad.

As a result of this bug we opted not to go past v13.0.0

@huozhi huozhi added the Lazy Loading Related to Next.js Lazy Loading (e.g., `next/dynamic` or `React.lazy`). label Feb 3, 2023
@kodiakhq kodiakhq bot closed this as completed in #45565 Feb 4, 2023
kodiakhq bot pushed a commit that referenced this issue Feb 4, 2023
## 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

github-actions bot commented Mar 6, 2023

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 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue was opened via the bug report template. Lazy Loading Related to Next.js Lazy Loading (e.g., `next/dynamic` or `React.lazy`).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants