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

Enable pure client suspense in blocking rendering #28165

Merged
merged 3 commits into from Aug 16, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion packages/next/shared/lib/dynamic.tsx
Expand Up @@ -122,7 +122,6 @@ export default function dynamic<P = {}>(
`Disallowed suspense option usage with next/dynamic in blocking mode`
)
}
suspenseOptions.suspense = false
}
if (suspenseOptions.suspense) {
return loadableFn(suspenseOptions)
Expand Down
13 changes: 11 additions & 2 deletions test/integration/react-18/app/pages/suspense/unwrapped.js
@@ -1,10 +1,19 @@
import React from 'react'
import React, { Suspense } from 'react'
import dynamic from 'next/dynamic'

// flag for testing
const wrapped = true

const Hello = dynamic(() => import('../../components/hello'), {
suspense: true,
})

export default function Unwrapped() {
return <Hello />
if (!wrapped) return <Hello />

return (
<Suspense fallback={`loading`}>
<Hello />
</Suspense>
)
}
4 changes: 2 additions & 2 deletions test/integration/react-18/test/basics.js
Expand Up @@ -2,9 +2,9 @@

import webdriver from 'next-webdriver'
import cheerio from 'cheerio'
import { fetchViaHTTP, renderViaHTTP } from 'next-test-utils'
import { fetchViaHTTP, renderViaHTTP, File, findPort } from 'next-test-utils'

export default (context) => {
export default (context, render) => {
it('hydrates correctly for normal page', async () => {
const browser = await webdriver(context.appPort, '/')
expect(await browser.eval('window.didHydrate')).toBe(true)
Expand Down
4 changes: 1 addition & 3 deletions test/integration/react-18/test/blocking.js
Expand Up @@ -20,8 +20,6 @@ export default (context, render) => {
const $ = await get$('/suspense/thrown')
const html = $('body').html()
expect(html).toContain('loading')
expect(
JSON.parse($('#__NEXT_DATA__').text()).dynamicIds
).not.toBeUndefined()
expect(JSON.parse($('#__NEXT_DATA__').text()).dynamicIds).toBeUndefined()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the only core change is here, using suspense + react.lazy won't have preload ids like loadable components.

})
}
55 changes: 34 additions & 21 deletions test/integration/react-18/test/index.test.js
Expand Up @@ -23,9 +23,8 @@ const nodeArgs = ['-r', join(__dirname, 'require-hook.js')]
const appDir = join(__dirname, '../app')
const nextConfig = new File(join(appDir, 'next.config.js'))
const dynamicHello = new File(join(appDir, 'components/dynamic-hello.js'))
const unwrappedPage = new File(join(appDir, 'pages/suspense/unwrapped.js'))

const SUSPENSE_ERROR_MESSAGE =
'Disallowed suspense option usage with next/dynamic'
const UNSUPPORTED_PRERELEASE =
"You are using an unsupported prerelease of 'react-dom'"
const USING_CREATE_ROOT = 'Using the createRoot API for React'
Expand Down Expand Up @@ -81,12 +80,15 @@ describe('React 18 Support', () => {
expect(output).not.toMatch(UNSUPPORTED_PRERELEASE)
})

it('suspense is not allowed in blocking rendering mode', async () => {
const appPort = await findPort()
const app = await launchApp(appDir, appPort)
const html = await renderViaHTTP(appPort, '/suspense/unwrapped')
await killApp(app)
expect(html).toContain(SUSPENSE_ERROR_MESSAGE)
test('suspense is not allowed in blocking rendering mode (prod)', async () => {
const { stderr, code } = await nextBuild(appDir, [], {
nodeArgs,
stderr: true,
})
expect(code).toBe(1)
expect(stderr).toContain(
'Disallowed suspense option usage with next/dynamic'
)
})
})

Expand Down Expand Up @@ -118,10 +120,23 @@ describe('React 18 Support', () => {
})

describe('Basics', () => {
runTests('default setting with react 18', 'dev', (context) => basics(context))
runTests('default setting with react 18', 'prod', (context) =>
basics(context)
runTests('default setting with react 18', (context) =>
basics(context, (p, q) => renderViaHTTP(context.appPort, p, q))
)

it('suspense is not allowed in blocking rendering mode (dev)', async () => {
// set dynamic.suspense = true but not wrapping with <Suspense>
unwrappedPage.replace('wrapped = true', 'wrapped = false')
const appPort = await findPort()
const app = await launchApp(appDir, appPort, { nodeArgs })
const html = await renderViaHTTP(appPort, '/suspense/unwrapped')
unwrappedPage.restore()
await killApp(app)
// expect(html).toContain('Disallowed suspense option usage with next/dynamic')
expect(html).toContain(
'A React component suspended while rendering, but no fallback UI was specified'
)
})
})

describe('Blocking mode', () => {
Expand All @@ -132,11 +147,7 @@ describe('Blocking mode', () => {
dynamicHello.restore()
})

runTests('concurrentFeatures is disabled', 'dev', (context) =>
blocking(context, (p, q) => renderViaHTTP(context.appPort, p, q))
)

runTests('concurrentFeatures is disabled', 'prod', (context) =>
runTests('concurrentFeatures is disabled', (context) =>
blocking(context, (p, q) => renderViaHTTP(context.appPort, p, q))
)
})
Expand All @@ -156,15 +167,12 @@ describe('Concurrent mode', () => {
dynamicHello.restore()
})

runTests('concurrentFeatures is enabled', 'dev', (context) =>
concurrent(context, (p, q) => renderViaHTTP(context.appPort, p, q))
)
runTests('concurrentFeatures is enabled', 'prod', (context) =>
runTests('concurrentFeatures is enabled', (context) =>
concurrent(context, (p, q) => renderViaHTTP(context.appPort, p, q))
)
})

function runTests(name, mode, fn) {
function runTest(mode, name, fn) {
const context = { appDir }
describe(`${name} (${mode})`, () => {
beforeAll(async () => {
Expand All @@ -186,3 +194,8 @@ function runTests(name, mode, fn) {
fn(context)
})
}

function runTests(name, fn) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

other test changes are just reflected to this simplification

runTest('dev', name, fn)
runTest('prod', name, fn)
}