Skip to content

Change page export validity check on client and server in development #5857

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

Merged
merged 17 commits into from
Dec 17, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions packages/next-server/lib/router/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -260,8 +260,11 @@ export default class Router {

const { Component } = routeInfo

if (typeof Component !== 'function') {
throw new Error(`The default export is not a React Component in page: "${pathname}"`)
if (process.env.NODE_ENV !== 'production') {
const { isValidElementType } = require('react-is')
if (!isValidElementType(Component)) {
throw new Error(`The default export is not a React Component in page: "${pathname}"`)
}
}

const ctx = { pathname, query, asPath: as }
Expand Down
1 change: 1 addition & 0 deletions packages/next-server/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
"@taskr/watch": "1.1.0",
"@types/react": "16.7.13",
"@types/react-dom": "16.0.11",
"@types/react-is": "16.5.0",
"@types/send": "0.14.4",
"taskr": "1.1.0",
"typescript": "3.1.6"
Expand Down
16 changes: 10 additions & 6 deletions packages/next-server/server/render.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ function renderDocument(Document: React.ComponentType, {
files={files}
dynamicImports={dynamicImports}
assetPrefix={assetPrefix}
{...docProps}
{...docProps}
/>
)
}
Expand All @@ -131,17 +131,21 @@ export async function renderToHTML (req: IncomingMessage, res: ServerResponse, p

await Loadable.preloadAll() // Make sure all dynamic imports are loaded

if (typeof Component !== 'function') {
throw new Error(`The default export is not a React Component in page: "${pathname}"`)
if (dev) {
const { isValidElementType } = require('react-is')
if (!isValidElementType(Component)) {
throw new Error(`The default export is not a React Component in page: "${pathname}"`)
}
}

if (!Document.prototype || !Document.prototype.isReactComponent) throw new Error('_document.js is not exporting a React component')

const asPath = req.url
const ctx = { err, req, res, pathname, query, asPath }
const router = new Router(pathname, query, asPath)
const props = await loadGetInitialProps(App, {Component, router, ctx})

// the response might be finshed on the getInitialProps call
// the response might be finished on the getInitialProps call
if (isResSent(res)) return null

const devFiles = buildManifest.devFiles
Expand All @@ -163,14 +167,14 @@ export async function renderToHTML (req: IncomingMessage, res: ServerResponse, p
return render(renderElementToString, <ErrorDebug error={err} />)
}

return render(renderElementToString,
return render(renderElementToString,
<LoadableCapture report={(moduleName) => reactLoadableModules.push(moduleName)}>
<EnhancedApp
Component={EnhancedComponent}
router={router}
{...props}
/>
</LoadableCapture>
</LoadableCapture>
)
}

Expand Down
7 changes: 5 additions & 2 deletions packages/next/client/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,11 @@ export default async ({
try {
Component = await pageLoader.loadPage(page)

if (typeof Component !== 'function') {
throw new Error(`The default export is not a React Component in page: "${page}"`)
if (process.env.NODE_ENV !== 'production') {
const { isValidElementType } = require('react-is')
if (!isValidElementType(Component)) {
throw new Error(`The default export is not a React Component in page: "${page}"`)
}
}
} catch (error) {
// This catches errors like throwing in the top level of a module
Expand Down
1 change: 1 addition & 0 deletions packages/next/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@
"prop-types": "15.6.2",
"prop-types-exact": "1.2.0",
"react-error-overlay": "4.0.0",
"react-is": "16.6.3",
"recursive-copy": "2.0.6",
"resolve": "1.5.0",
"strip-ansi": "3.0.1",
Expand Down
7 changes: 7 additions & 0 deletions test/integration/basic/pages/forwardRef-component.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import React from 'react'

export default React.forwardRef((props, ref) => (
<span {...props} forwardedRef={ref}>
This is a component with a forwarded ref
</span>
))
3 changes: 3 additions & 0 deletions test/integration/basic/pages/memo-component.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import React from 'react'

export default React.memo((props) => <span {...props}>Memo component</span>)
4 changes: 2 additions & 2 deletions test/integration/basic/test/error-recovery.js
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ export default (context) => {
const text = await browser.elementByCss('p').text()
expect(text).toBe('This is the about page.')

aboutPage.replace('export default', 'export default "not-a-page"\nexport const fn = ')
aboutPage.replace('export default', 'export default {};\nexport const fn =')

await check(
() => getBrowserBodyText(browser),
Expand Down Expand Up @@ -250,7 +250,7 @@ export default (context) => {
const text = await browser.elementByCss('p').text()
expect(text).toBe('This is the about page.')

aboutPage.replace('export default', 'export default () => /search/ \nexport const fn = ')
aboutPage.replace('export default', 'export default () => /search/;\nexport const fn =')

await check(
() => getBrowserBodyText(browser),
Expand Down
10 changes: 10 additions & 0 deletions test/integration/basic/test/rendering.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,16 @@ export default function ({ app }, suiteName, render, fetch) {
expect(html.includes('My component!')).toBeTruthy()
})

test('renders when component is a forwardRef instance', async () => {
const html = await render('/forwardRef-component')
expect(html.includes('This is a component with a forwarded ref')).toBeTruthy()
})

test('renders when component is a memo instance', async () => {
const html = await render('/memo-component')
expect(html.includes('Memo component')).toBeTruthy()
})

// default-head contains an empty <Head />.
test('header renders default charset', async () => {
const html = await (render('/default-head'))
Expand Down
9 changes: 8 additions & 1 deletion yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -1433,6 +1433,13 @@
dependencies:
"@types/react" "*"

"@types/react-is@16.5.0":
version "16.5.0"
resolved "https://registry.yarnpkg.com/@types/react-is/-/react-is-16.5.0.tgz#6b0dd43e60fa7c82b48faf7b487543079a61015a"
integrity sha512-yUYPioB2Sh5d4csgpW/vJwxWM0RG1/QbGiwYap2m/bEAQKRwbagYRc5C7oK2AM9QC2vr2ZViCgpm0DpDpFQ6XA==
dependencies:
"@types/react" "*"

"@types/react@*", "@types/react@16.7.13":
version "16.7.13"
resolved "https://registry.yarnpkg.com/@types/react/-/react-16.7.13.tgz#d2369ae78377356d42fb54275d30218e84f2247a"
Expand Down Expand Up @@ -9245,7 +9252,7 @@ react-error-overlay@4.0.0:
resolved "https://registry.yarnpkg.com/react-error-overlay/-/react-error-overlay-4.0.0.tgz#d198408a85b4070937a98667f500c832f86bd5d4"
integrity sha512-FlsPxavEyMuR6TjVbSSywovXSEyOg6ZDj5+Z8nbsRl9EkOzAhEIcS+GLoQDC5fz/t9suhUXWmUrOBrgeUvrMxw==

react-is@^16.3.2:
react-is@16.6.3, react-is@^16.3.2:
version "16.6.3"
resolved "https://registry.yarnpkg.com/react-is/-/react-is-16.6.3.tgz#d2d7462fcfcbe6ec0da56ad69047e47e56e7eac0"
integrity sha512-u7FDWtthB4rWibG/+mFbVd5FvdI20yde86qKGx4lVUTWmPlSWQ4QxbBIrrs+HnXGbxOUlUzTAP/VDmvCwaP2yA==
Expand Down