From 72e7929242f20526d76ef7884b9000852bfe4371 Mon Sep 17 00:00:00 2001 From: Kyle Holmberg Date: Mon, 17 Dec 2018 07:09:23 -0800 Subject: [PATCH] Change page export validity check on client and server in development (#5857) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves #4055 Credit: https://github.com/zeit/next.js/pull/5095 I didn't use the ignore webpack plugin from the original PR and tested bundle size with https://github.com/zeit/next.js/pull/5339 - seems to be safe on that front. Was able to get tests to pass locally, unsure of what goes wrong in CI 🤷‍♂️ **Questions** 1) The initial PR didn't include changes to `next-server/lib/router` in `getRouteInfo()`. Should the same changes be made within? 2) Should we add a test for rendering a component created via `forwardRef()`? `component-with-forwardedRef`: ```javascript export default React.forwardRef((props, ref) => This is a component with a forwarded ref); ``` some test: ```javascript test('renders from forwardRef', async () => { const $ = await get$('/component-with-forwardedRef') const span = $('span') expect(span.text()).toMatch(/This is a component with a forwarded ref/) }) ``` --- packages/next-server/lib/router/router.js | 7 +++++-- packages/next-server/package.json | 1 + packages/next-server/server/render.tsx | 16 ++++++++++------ packages/next/client/index.js | 7 +++++-- packages/next/package.json | 1 + .../basic/pages/forwardRef-component.js | 7 +++++++ test/integration/basic/pages/memo-component.js | 3 +++ test/integration/basic/test/error-recovery.js | 4 ++-- test/integration/basic/test/rendering.js | 10 ++++++++++ yarn.lock | 9 ++++++++- 10 files changed, 52 insertions(+), 13 deletions(-) create mode 100644 test/integration/basic/pages/forwardRef-component.js create mode 100644 test/integration/basic/pages/memo-component.js diff --git a/packages/next-server/lib/router/router.js b/packages/next-server/lib/router/router.js index ee48b2f5..82aa924b 100644 --- a/packages/next-server/lib/router/router.js +++ b/packages/next-server/lib/router/router.js @@ -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 } diff --git a/packages/next-server/package.json b/packages/next-server/package.json index 355fc4f5..82957231 100644 --- a/packages/next-server/package.json +++ b/packages/next-server/package.json @@ -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" diff --git a/packages/next-server/server/render.tsx b/packages/next-server/server/render.tsx index e16907a6..c328cf55 100644 --- a/packages/next-server/server/render.tsx +++ b/packages/next-server/server/render.tsx @@ -106,7 +106,7 @@ function renderDocument(Document: React.ComponentType, { files={files} dynamicImports={dynamicImports} assetPrefix={assetPrefix} - {...docProps} + {...docProps} /> ) } @@ -131,9 +131,13 @@ 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 @@ -141,7 +145,7 @@ export async function renderToHTML (req: IncomingMessage, res: ServerResponse, p 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 @@ -163,14 +167,14 @@ export async function renderToHTML (req: IncomingMessage, res: ServerResponse, p return render(renderElementToString, ) } - return render(renderElementToString, + return render(renderElementToString, reactLoadableModules.push(moduleName)}> - + ) } diff --git a/packages/next/client/index.js b/packages/next/client/index.js index 892bb935..737c42e1 100644 --- a/packages/next/client/index.js +++ b/packages/next/client/index.js @@ -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 diff --git a/packages/next/package.json b/packages/next/package.json index 9912ee40..d6f494e8 100644 --- a/packages/next/package.json +++ b/packages/next/package.json @@ -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", diff --git a/test/integration/basic/pages/forwardRef-component.js b/test/integration/basic/pages/forwardRef-component.js new file mode 100644 index 00000000..144abe74 --- /dev/null +++ b/test/integration/basic/pages/forwardRef-component.js @@ -0,0 +1,7 @@ +import React from 'react' + +export default React.forwardRef((props, ref) => ( + + This is a component with a forwarded ref + +)) diff --git a/test/integration/basic/pages/memo-component.js b/test/integration/basic/pages/memo-component.js new file mode 100644 index 00000000..6bb12c46 --- /dev/null +++ b/test/integration/basic/pages/memo-component.js @@ -0,0 +1,3 @@ +import React from 'react' + +export default React.memo((props) => Memo component) diff --git a/test/integration/basic/test/error-recovery.js b/test/integration/basic/test/error-recovery.js index c080541e..57b50a9d 100644 --- a/test/integration/basic/test/error-recovery.js +++ b/test/integration/basic/test/error-recovery.js @@ -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), @@ -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), diff --git a/test/integration/basic/test/rendering.js b/test/integration/basic/test/rendering.js index a2381afc..e370ef49 100644 --- a/test/integration/basic/test/rendering.js +++ b/test/integration/basic/test/rendering.js @@ -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 . test('header renders default charset', async () => { const html = await (render('/default-head')) diff --git a/yarn.lock b/yarn.lock index 28255b0c..22c66346 100644 --- a/yarn.lock +++ b/yarn.lock @@ -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" @@ -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==