Remove extra suspense boundary for default next/dynamic (#67014)
### What Reland #64716 Removing the Suspense boundary on top of `next/dynamic` by default, make it as `React.lazy` component with preloading CSS feature. * Remove `suspense` option in `next/dynamic` since it's already deprecated for a while * Remove the default loading in app router implmentation of `next/dynamic` ### Why Extra Suspense boundary is causing extra useless rendering. For SSR, it shouldn't render `loading` by default Related: #64060 Related: #64687 Closes [NEXT-3074](https://linear.app/vercel/issue/NEXT-3074/app-router-content-flickering-with-reactcreatecontext-and-nextdynamic) This is sort of a breaking change, since removing the Suspense boundary on top of `next/dynamic` by default. If there's error happening in side the dynamic component you need to wrap an extra Suspense boundary on top of it
This commit is contained in:
parent
cfacca5fad
commit
1a04d94aae
13 changed files with 115 additions and 48 deletions
|
@ -1,4 +1,5 @@
|
|||
import React, { type JSX } from 'react'
|
||||
import type React from 'react'
|
||||
import type { JSX } from 'react'
|
||||
import Loadable from './lazy-dynamic/loadable'
|
||||
|
||||
import type {
|
||||
|
@ -16,7 +17,7 @@ export {
|
|||
}
|
||||
|
||||
export type DynamicOptions<P = {}> = LoadableGeneratedOptions & {
|
||||
loading?: (loadingProps: DynamicOptionsLoadingProps) => JSX.Element | null
|
||||
loading?: () => JSX.Element | null
|
||||
loader?: Loader<P>
|
||||
loadableGenerated?: LoadableGeneratedOptions
|
||||
modules?: string[]
|
||||
|
@ -35,27 +36,7 @@ export default function dynamic<P = {}>(
|
|||
dynamicOptions: DynamicOptions<P> | Loader<P>,
|
||||
options?: DynamicOptions<P>
|
||||
): React.ComponentType<P> {
|
||||
let loadableOptions: LoadableOptions<P> = {
|
||||
// A loading component is not required, so we default it
|
||||
loading: ({ error, isLoading, pastDelay }) => {
|
||||
if (!pastDelay) return null
|
||||
if (process.env.NODE_ENV !== 'production') {
|
||||
if (isLoading) {
|
||||
return null
|
||||
}
|
||||
if (error) {
|
||||
return (
|
||||
<p>
|
||||
{error.message}
|
||||
<br />
|
||||
{error.stack}
|
||||
</p>
|
||||
)
|
||||
}
|
||||
}
|
||||
return null
|
||||
},
|
||||
}
|
||||
const loadableOptions: LoadableOptions<P> = {}
|
||||
|
||||
if (typeof dynamicOptions === 'function') {
|
||||
loadableOptions.loader = dynamicOptions
|
||||
|
|
|
@ -40,10 +40,6 @@ export type DynamicOptions<P = {}> = LoadableGeneratedOptions & {
|
|||
loader?: Loader<P> | LoaderMap
|
||||
loadableGenerated?: LoadableGeneratedOptions
|
||||
ssr?: boolean
|
||||
/**
|
||||
* @deprecated `suspense` prop is not required anymore
|
||||
*/
|
||||
suspense?: boolean
|
||||
}
|
||||
|
||||
export type LoadableOptions<P = {}> = DynamicOptions<P>
|
||||
|
|
|
@ -1,4 +1,4 @@
|
|||
import { Suspense, lazy } from 'react'
|
||||
import { Suspense, Fragment, lazy } from 'react'
|
||||
import { BailoutToCSR } from './dynamic-bailout-to-csr'
|
||||
import type { ComponentModule } from './types'
|
||||
import { PreloadChunks } from './preload-chunks'
|
||||
|
@ -48,6 +48,10 @@ function Loadable(options: LoadableOptions) {
|
|||
<Loading isLoading={true} pastDelay={true} error={null} />
|
||||
) : null
|
||||
|
||||
// If it's non-SSR or provided a loading component, wrap it in a suspense boundary
|
||||
const hasSuspenseBoundary = !opts.ssr || !!opts.loading
|
||||
const Wrap = hasSuspenseBoundary ? Suspense : Fragment
|
||||
const wrapProps = hasSuspenseBoundary ? { fallback: fallbackElement } : {}
|
||||
const children = opts.ssr ? (
|
||||
<>
|
||||
{/* During SSR, we need to preload the CSS from the dynamic component to avoid flash of unstyled content */}
|
||||
|
@ -62,7 +66,7 @@ function Loadable(options: LoadableOptions) {
|
|||
</BailoutToCSR>
|
||||
)
|
||||
|
||||
return <Suspense fallback={fallbackElement}>{children}</Suspense>
|
||||
return <Wrap {...wrapProps}>{children}</Wrap>
|
||||
}
|
||||
|
||||
LoadableComponent.displayName = 'LoadableComponent'
|
||||
|
|
|
@ -0,0 +1,12 @@
|
|||
const isDevTest = false
|
||||
|
||||
const DynamicImportComponent = () => {
|
||||
if (isDevTest && typeof window === 'undefined') {
|
||||
throw new Error('This component should only be rendered on the client side')
|
||||
}
|
||||
return (
|
||||
<div id="dynamic-component">This is a dynamically imported component</div>
|
||||
)
|
||||
}
|
||||
|
||||
export default DynamicImportComponent
|
26
test/e2e/app-dir/dynamic/app/default-loading/page.js
Normal file
26
test/e2e/app-dir/dynamic/app/default-loading/page.js
Normal file
|
@ -0,0 +1,26 @@
|
|||
'use client'
|
||||
|
||||
import dynamic from 'next/dynamic'
|
||||
|
||||
const DynamicHeader = dynamic(
|
||||
() => {
|
||||
return new Promise((resolve) => {
|
||||
setTimeout(() => {
|
||||
resolve(import('./dynamic-component'))
|
||||
}, 1000)
|
||||
})
|
||||
},
|
||||
{
|
||||
loading: () => <p>Loading...</p>,
|
||||
}
|
||||
)
|
||||
|
||||
const Page = () => {
|
||||
return (
|
||||
<div>
|
||||
<DynamicHeader />
|
||||
</div>
|
||||
)
|
||||
}
|
||||
|
||||
export default Page
|
|
@ -0,0 +1,7 @@
|
|||
const DynamicImportComponent = () => {
|
||||
return (
|
||||
<div id="dynamic-component">This is a dynamically imported component</div>
|
||||
)
|
||||
}
|
||||
|
||||
export default DynamicImportComponent
|
21
test/e2e/app-dir/dynamic/app/default/page.js
Normal file
21
test/e2e/app-dir/dynamic/app/default/page.js
Normal file
|
@ -0,0 +1,21 @@
|
|||
'use client'
|
||||
|
||||
import dynamic from 'next/dynamic'
|
||||
|
||||
const DynamicHeader = dynamic(() => {
|
||||
return new Promise((resolve) => {
|
||||
setTimeout(() => {
|
||||
resolve(import('./dynamic-component'))
|
||||
}, 1000)
|
||||
})
|
||||
})
|
||||
|
||||
const Page = () => {
|
||||
return (
|
||||
<div>
|
||||
<DynamicHeader />
|
||||
</div>
|
||||
)
|
||||
}
|
||||
|
||||
export default Page
|
|
@ -1,7 +1,8 @@
|
|||
import { nextTestSetup } from 'e2e-utils'
|
||||
import { retry } from 'next-test-utils'
|
||||
|
||||
describe('app dir - next/dynamic', () => {
|
||||
const { next, isNextStart, skipped } = nextTestSetup({
|
||||
const { next, isNextStart, isNextDev, skipped } = nextTestSetup({
|
||||
files: __dirname,
|
||||
skipDeployment: true,
|
||||
})
|
||||
|
@ -57,6 +58,35 @@ describe('app dir - next/dynamic', () => {
|
|||
expect($('h1').text()).toBe('hello')
|
||||
})
|
||||
|
||||
it('should render loading by default if loading is specified and loader is slow', async () => {
|
||||
const $ = await next.render$('/default-loading')
|
||||
|
||||
// First render in dev should show loading, production build will resolve the content.
|
||||
expect($('body').text()).toContain(
|
||||
isNextDev ? 'Loading...' : 'This is a dynamically imported component'
|
||||
)
|
||||
})
|
||||
|
||||
it('should not render loading by default', async () => {
|
||||
const $ = await next.render$('/default')
|
||||
expect($('#dynamic-component').text()).not.toContain('loading')
|
||||
})
|
||||
|
||||
if (isNextDev) {
|
||||
it('should directly raise error when dynamic component error on server', async () => {
|
||||
const pagePath = 'app/default-loading/dynamic-component.js'
|
||||
const page = await next.readFile(pagePath)
|
||||
await next.patchFile(
|
||||
pagePath,
|
||||
page.replace('const isDevTest = false', 'const isDevTest = true')
|
||||
)
|
||||
await retry(async () => {
|
||||
const { status } = await next.fetch('/default-loading')
|
||||
expect(status).toBe(200)
|
||||
})
|
||||
})
|
||||
}
|
||||
|
||||
describe('no SSR', () => {
|
||||
it('should not render client component imported through ssr: false in client components in edge runtime', async () => {
|
||||
// noSSR should not show up in html
|
||||
|
|
|
@ -1,8 +1,13 @@
|
|||
'use client'
|
||||
|
||||
import dynamic from 'next/dynamic'
|
||||
import { Suspense } from 'react'
|
||||
const Component = dynamic(() => import('./component'))
|
||||
|
||||
export default async function Inner() {
|
||||
return <Component />
|
||||
return (
|
||||
<Suspense>
|
||||
<Component />
|
||||
</Suspense>
|
||||
)
|
||||
}
|
||||
|
|
|
@ -40,7 +40,7 @@ describe('next-dynamic-css', () => {
|
|||
})
|
||||
}
|
||||
|
||||
it('should have correct order of styles on next/dymamic loaded component', async () => {
|
||||
it('should have correct order of styles on next/dynamic loaded component', async () => {
|
||||
const browser = await next.browser('/page')
|
||||
expect(await browser.waitForElementByCss('#component').text()).toBe(
|
||||
'Hello Component'
|
||||
|
|
|
@ -1,12 +0,0 @@
|
|||
import dynamic from 'next/dynamic'
|
||||
import { Suspense } from 'react'
|
||||
|
||||
const Foo = dynamic(() => import('../components/foo'), { suspense: true })
|
||||
|
||||
export default () => (
|
||||
<div>
|
||||
<Suspense fallback="fallback">
|
||||
<Foo />
|
||||
</Suspense>
|
||||
</div>
|
||||
)
|
|
@ -3,9 +3,7 @@
|
|||
import React from 'react'
|
||||
import dynamic from 'next/dynamic'
|
||||
|
||||
const Red = dynamic(() => import('../../components/red'), {
|
||||
suspense: true,
|
||||
})
|
||||
const Red = dynamic(() => import('../../components/red'))
|
||||
|
||||
function Blue() {
|
||||
return (
|
||||
|
|
|
@ -66,7 +66,6 @@ describe('Basics', () => {
|
|||
}
|
||||
}
|
||||
await expectToContainPreload('dynamic')
|
||||
await expectToContainPreload('dynamic-suspense')
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
Loading…
Reference in a new issue