Skip to content

Commit

Permalink
Leverage existing component checking warning for streaming (#34526)
Browse files Browse the repository at this point in the history
## Bug

Fixes: #31993

* Remove the simple component checking in middleware ssr
* Leverage existing components checking for Component / App / Document, if any of these component is not valid react type or is undefined nextjs will error in dev mode with redbox. Like above.

<img width="826" alt="image" src="https://user-images.githubusercontent.com/4800338/154668945-bcee24ee-17aa-4afd-acda-9f8b249891ac.png">


- [x] Related issues linked using `fixes #number`
- [x] Integration tests added
- [x] Errors have helpful link attached, see `contributing.md`
  • Loading branch information
huozhi committed Feb 18, 2022
1 parent 4ad1c5a commit ce76d17
Show file tree
Hide file tree
Showing 5 changed files with 56 additions and 31 deletions.
Expand Up @@ -39,10 +39,6 @@ export default async function middlewareSSRLoader(this: any) {
const reactLoadableManifest = self.__REACT_LOADABLE_MANIFEST
const rscManifest = self.__RSC_MANIFEST
if (typeof pageMod.default !== 'function') {
throw new Error('Your page must export a \`default\` component')
}
// Set server context
self.__server_context = {
page: ${JSON.stringify(page)},
Expand Down
2 changes: 1 addition & 1 deletion packages/next/server/base-server.ts
Expand Up @@ -1122,7 +1122,7 @@ export default abstract class Server {
const isSSG = !!components.getStaticProps
const hasServerProps = !!components.getServerSideProps
const hasStaticPaths = !!components.getStaticPaths
const hasGetInitialProps = !!(components.Component as any).getInitialProps
const hasGetInitialProps = !!components.Component?.getInitialProps

// Toggle whether or not this is a Data request
const isDataReq = !!query._nextDataReq && (isSSG || hasServerProps)
Expand Down
8 changes: 6 additions & 2 deletions packages/next/server/load-components.ts
@@ -1,11 +1,15 @@
import type {
AppType,
DocumentType,
NextComponentType,
} from '../shared/lib/utils'
import {
BUILD_MANIFEST,
REACT_LOADABLE_MANIFEST,
} from '../shared/lib/constants'
import { join } from 'path'
import { requirePage } from './require'
import { BuildManifest } from './get-page-files'
import { AppType, DocumentType } from '../shared/lib/utils'
import { interopDefault } from '../lib/interop-default'
import {
PageConfig,
Expand All @@ -22,7 +26,7 @@ export type ManifestItem = {
export type ReactLoadableManifest = { [moduleId: string]: ManifestItem }

export type LoadComponentsReturnType = {
Component: React.ComponentType
Component: NextComponentType
pageConfig: PageConfig
buildManifest: BuildManifest
reactLoadableManifest: ReactLoadableManifest
Expand Down
4 changes: 2 additions & 2 deletions packages/next/server/render.tsx
Expand Up @@ -545,7 +545,7 @@ export async function renderToHTML(
const defaultAppGetInitialProps =
App.getInitialProps === (App as any).origGetInitialProps

const hasPageGetInitialProps = !!(Component as any).getInitialProps
const hasPageGetInitialProps = !!(Component as any)?.getInitialProps

const pageIsDynamic = isDynamicRoute(pathname)

Expand All @@ -561,7 +561,7 @@ export async function renderToHTML(
'getServerSideProps',
'getStaticPaths',
]) {
if ((Component as any)[methodName]) {
if ((Component as any)?.[methodName]) {
throw new Error(
`page ${pathname} ${methodName} ${GSSP_COMPONENT_MEMBER_ERROR}`
)
Expand Down
69 changes: 47 additions & 22 deletions test/integration/react-18/test/index.test.js
Expand Up @@ -11,18 +11,22 @@ import {
nextStart,
renderViaHTTP,
fetchViaHTTP,
hasRedbox,
getRedboxHeader,
} from 'next-test-utils'
import blocking from './blocking'
import concurrent from './concurrent'
import basics from './basics'
import strictMode from './strict-mode'
import webdriver from 'next-webdriver'

// overrides react and react-dom to v18
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 invalidPage = new File(join(appDir, 'pages/invalid.js'))

const USING_CREATE_ROOT = 'Using the createRoot API for React'

Expand Down Expand Up @@ -141,21 +145,22 @@ describe('Blocking mode', () => {
})

function runTestsAgainstRuntime(runtime) {
describe(`Concurrent mode in the ${runtime} runtime`, () => {
beforeAll(async () => {
nextConfig.replace("// runtime: 'edge'", `runtime: '${runtime}'`)
dynamicHello.replace('suspense = false', `suspense = true`)
// `noSSR` mode will be ignored by suspense
dynamicHello.replace('let ssr', `let ssr = false`)
})
afterAll(async () => {
nextConfig.restore()
dynamicHello.restore()
})

runTests(`runtime is set to '${runtime}'`, (context) => {
runTests(
`Concurrent mode in the ${runtime} runtime`,
(context, env) => {
concurrent(context, (p, q) => renderViaHTTP(context.appPort, p, q))

if (env === 'dev') {
it('should recover after undefined exported as default', async () => {
const browser = await webdriver(context.appPort, '/invalid')

expect(await hasRedbox(browser)).toBe(true)
expect(await getRedboxHeader(browser)).toMatch(
`Error: The default export is not a React Component in page: "/invalid"`
)
})
}

it('should stream to users', async () => {
const res = await fetchViaHTTP(context.appPort, '/ssr')
expect(res.headers.get('etag')).toBeNull()
Expand Down Expand Up @@ -189,17 +194,36 @@ function runTestsAgainstRuntime(runtime) {
)
expect(res.headers.get('etag')).toBeDefined()
})
})
})
},
{
beforeAll: (env) => {
if (env === 'dev') {
invalidPage.write(`export const value = 1`)
}
nextConfig.replace("// runtime: 'edge'", `runtime: '${runtime}'`)
dynamicHello.replace('suspense = false', `suspense = true`)
// `noSSR` mode will be ignored by suspense
dynamicHello.replace('let ssr', `let ssr = false`)
},
afterAll: (env) => {
if (env === 'dev') {
invalidPage.delete()
}
nextConfig.restore()
dynamicHello.restore()
},
}
)
}

function runTest(mode, name, fn) {
function runTest(env, name, fn, options) {
const context = { appDir }
describe(`${name} (${mode})`, () => {
describe(`${name} (${env})`, () => {
beforeAll(async () => {
context.appPort = await findPort()
context.stderr = ''
if (mode === 'dev') {
options?.beforeAll(env)
if (env === 'dev') {
context.server = await launchApp(context.appDir, context.appPort, {
nodeArgs,
onStderr(msg) {
Expand All @@ -217,16 +241,17 @@ function runTest(mode, name, fn) {
}
})
afterAll(async () => {
options?.afterAll(env)
await killApp(context.server)
})
fn(context)
fn(context, env)
})
}

runTestsAgainstRuntime('edge')
runTestsAgainstRuntime('nodejs')

function runTests(name, fn) {
runTest('dev', name, fn)
runTest('prod', name, fn)
function runTests(name, fn, options) {
runTest('dev', name, fn, options)
runTest('prod', name, fn, options)
}

0 comments on commit ce76d17

Please sign in to comment.