From c8da345765e1e96b98a1625acfd70470956505de Mon Sep 17 00:00:00 2001 From: Tim Neutkens Date: Tue, 13 Feb 2018 14:27:52 +0100 Subject: [PATCH] Make page require faster (#3776) * Make page require faster * Add windows search/replace * Use normalize instead of resolve * Add remaining tests * Use sep instead of / * Add test files * Make component require faster * Add console.error --- server/render.js | 14 ++-- server/require.js | 66 ++++++++++++++- test/isolated/_resolvedata/.gitignore | 1 + .../_resolvedata/dist/bundles/pages/index.js | 3 + .../_resolvedata/dist/bundles/pages/world.js | 3 + test/isolated/require-page.test.js | 82 +++++++++++++++++++ 6 files changed, 157 insertions(+), 12 deletions(-) create mode 100644 test/isolated/_resolvedata/.gitignore create mode 100644 test/isolated/_resolvedata/dist/bundles/pages/index.js create mode 100644 test/isolated/_resolvedata/dist/bundles/pages/world.js create mode 100644 test/isolated/require-page.test.js diff --git a/server/render.js b/server/render.js index 6600c1e9..53316d4e 100644 --- a/server/render.js +++ b/server/render.js @@ -4,7 +4,7 @@ import { renderToString, renderToStaticMarkup } from 'react-dom/server' import send from 'send' import generateETag from 'etag' import fresh from 'fresh' -import requireModule from './require' +import requirePage from './require' import getConfig from './config' import { Router } from '../lib/router' import { loadGetInitialProps, isResSent } from '../lib/utils' @@ -48,17 +48,16 @@ async function doRender (req, res, pathname, query, { } = {}) { page = page || pathname - await ensurePage(page, { dir, hotReloader }) + if (hotReloader) { // In dev mode we use on demand entries to compile the page before rendering + await ensurePage(page, { dir, hotReloader }) + } const dist = getConfig(dir).distDir - const pagePath = join(dir, dist, 'dist', 'bundles', 'pages', page) const documentPath = join(dir, dist, 'dist', 'bundles', 'pages', '_document') - let [Component, Document] = await Promise.all([ - requireModule(pagePath), - requireModule(documentPath) - ]) + let Component = requirePage(page, {dir, dist}) + let Document = require(documentPath) Component = Component.default || Component Document = Document.default || Document const asPath = req.url @@ -225,7 +224,6 @@ export function serveStatic (req, res, path) { } async function ensurePage (page, { dir, hotReloader }) { - if (!hotReloader) return if (page === '_error' || page === '_document') return await hotReloader.ensurePage(page) diff --git a/server/require.js b/server/require.js index 3ca13f32..0388ee32 100644 --- a/server/require.js +++ b/server/require.js @@ -1,6 +1,64 @@ -import resolve from './resolve' +import {join, parse, normalize, sep} from 'path' -export default async function requireModule (path) { - const f = await resolve(path) - return require(f) +export function pageNotFoundError (page) { + const err = new Error(`Cannot find module for page: ${page}`) + err.code = 'ENOENT' + return err +} + +export function normalizePagePath (page) { + // If the page is `/` we need to append `/index`, otherwise the returned directory root will be bundles instead of pages + if (page === '/') { + page = '/index' + } + + // Resolve on anything that doesn't start with `/` + if (page[0] !== '/') { + page = `/${page}` + } + + // Windows compatibility + if (sep !== '/') { + page = page.replace(/\//g, sep) + } + + // Throw when using ../ etc in the pathname + const resolvedPage = normalize(page) + if (page !== resolvedPage) { + throw new Error('Requested and resolved page mismatch') + } + + return page +} + +export function getPagePath (page, {dir, dist}) { + const pageBundlesPath = join(dir, dist, 'dist', 'bundles', 'pages') + + try { + page = normalizePagePath(page) + } catch (err) { + console.error(err) + throw pageNotFoundError(page) + } + + const pagePath = join(pageBundlesPath, page) // Path to the page that is to be loaded + + // Don't allow wandering outside of the bundles directory + const pathDir = parse(pagePath).dir + if (pathDir.indexOf(pageBundlesPath) !== 0) { + console.error('Resolved page path goes outside of bundles path') + throw pageNotFoundError(page) + } + + return pagePath +} + +export default function requirePage (page, {dir, dist}) { + const pagePath = getPagePath(page, {dir, dist}) + try { + return require(pagePath) + } catch (err) { + console.error(err) + throw pageNotFoundError(page) + } } diff --git a/test/isolated/_resolvedata/.gitignore b/test/isolated/_resolvedata/.gitignore new file mode 100644 index 00000000..586e3d7a --- /dev/null +++ b/test/isolated/_resolvedata/.gitignore @@ -0,0 +1 @@ +!dist \ No newline at end of file diff --git a/test/isolated/_resolvedata/dist/bundles/pages/index.js b/test/isolated/_resolvedata/dist/bundles/pages/index.js new file mode 100644 index 00000000..30de59be --- /dev/null +++ b/test/isolated/_resolvedata/dist/bundles/pages/index.js @@ -0,0 +1,3 @@ +module.exports = { + test: 'hello' +} \ No newline at end of file diff --git a/test/isolated/_resolvedata/dist/bundles/pages/world.js b/test/isolated/_resolvedata/dist/bundles/pages/world.js new file mode 100644 index 00000000..24ca31a3 --- /dev/null +++ b/test/isolated/_resolvedata/dist/bundles/pages/world.js @@ -0,0 +1,3 @@ +module.exports = { + test: 'world' +} \ No newline at end of file diff --git a/test/isolated/require-page.test.js b/test/isolated/require-page.test.js new file mode 100644 index 00000000..e7e6e513 --- /dev/null +++ b/test/isolated/require-page.test.js @@ -0,0 +1,82 @@ +/* global describe, it, expect */ + +import { join, sep } from 'path' +import requirePage, {getPagePath, normalizePagePath, pageNotFoundError} from '../../dist/server/require' + +const dir = '/path/to/some/project' +const dist = '.next' + +const pathToBundles = join(dir, dist, 'dist', 'bundles', 'pages') + +describe('pageNotFoundError', () => { + it('Should throw error with ENOENT code', () => { + try { + pageNotFoundError('test') + } catch (err) { + expect(err.code).toBe('ENOENT') + } + }) +}) + +describe('normalizePagePath', () => { + it('Should turn / into /index', () => { + expect(normalizePagePath('/')).toBe(`${sep}index`) + }) + + it('Should turn _error into /_error', () => { + expect(normalizePagePath('_error')).toBe(`${sep}_error`) + }) + + it('Should turn /abc into /abc', () => { + expect(normalizePagePath('/abc')).toBe(`${sep}abc`) + }) + + it('Should turn /abc/def into /abc/def', () => { + expect(normalizePagePath('/abc/def')).toBe(`${sep}abc${sep}def`) + }) + + it('Should throw on /../../test.js', () => { + expect(() => normalizePagePath('/../../test.js')).toThrow() + }) +}) + +describe('getPagePath', () => { + it('Should append /index to the / page', () => { + const pagePath = getPagePath('/', {dir, dist}) + expect(pagePath).toBe(join(pathToBundles, `${sep}index`)) + }) + + it('Should prepend / when a page does not have it', () => { + const pagePath = getPagePath('_error', {dir, dist}) + expect(pagePath).toBe(join(pathToBundles, `${sep}_error`)) + }) + + it('Should throw with paths containing ../', () => { + expect(() => getPagePath('/../../package.json', {dir, dist})).toThrow() + }) +}) + +describe('requirePage', () => { + it('Should require /index.js when using /', () => { + const page = requirePage('/', {dir: __dirname, dist: '_resolvedata'}) + expect(page.test).toBe('hello') + }) + + it('Should require /index.js when using /index', () => { + const page = requirePage('/index', {dir: __dirname, dist: '_resolvedata'}) + expect(page.test).toBe('hello') + }) + + it('Should require /world.js when using /world', () => { + const page = requirePage('/world', {dir: __dirname, dist: '_resolvedata'}) + expect(page.test).toBe('world') + }) + + it('Should throw when using /../../test.js', () => { + expect(() => requirePage('/../../test.js', {dir: __dirname, dist: '_resolvedata'})).toThrow() + }) + + it('Should throw when using non existent pages like /non-existent.js', () => { + expect(() => requirePage('/non-existent.js', {dir: __dirname, dist: '_resolvedata'})).toThrow() + }) +})