From 7e9f4f9327e3db156db20db9548a367b3e64b66c Mon Sep 17 00:00:00 2001
From: Tim Neutkens <tim@timneutkens.nl>
Date: Fri, 9 Mar 2018 17:14:30 +0100
Subject: [PATCH] Only show 404 when the page does not exist (#3976)

* Only show 404 when the page does not exist

* Do async filesystem check
---
 server/render.js                              |  6 ++--
 server/require.js                             | 22 +++++-------
 .../dist/bundles/pages/non-existent-child.js  |  4 +++
 test/isolated/require-page.test.js            | 36 +++++++++++++------
 4 files changed, 42 insertions(+), 26 deletions(-)
 create mode 100644 test/isolated/_resolvedata/dist/bundles/pages/non-existent-child.js

diff --git a/server/render.js b/server/render.js
index 8b4b3fe0..0d868ac9 100644
--- a/server/render.js
+++ b/server/render.js
@@ -55,8 +55,10 @@ async function doRender (req, res, pathname, query, {
 
   const documentPath = join(dir, dist, 'dist', 'bundles', 'pages', '_document')
 
-  let Component = requirePage(page, {dir, dist})
-  let Document = require(documentPath)
+  let [Component, Document] = await Promise.all([
+    requirePage(page, {dir, dist}),
+    require(documentPath)
+  ])
   Component = Component.default || Component
   Document = Document.default || Document
   const asPath = req.url
diff --git a/server/require.js b/server/require.js
index c1dd480a..0ced446d 100644
--- a/server/require.js
+++ b/server/require.js
@@ -1,4 +1,5 @@
 import {join, parse, normalize, sep} from 'path'
+import fs from 'mz/fs'
 
 export function pageNotFoundError (page) {
   const err = new Error(`Cannot find module for page: ${page}`)
@@ -53,19 +54,12 @@ export function getPagePath (page, {dir, dist}) {
   return pagePath
 }
 
-export default function requirePage (page, {dir, dist}) {
-  const pagePath = getPagePath(page, {dir, dist})
-
-  try {
-    return require(pagePath)
-  } catch (err) {
-    if (err.code === 'MODULE_NOT_FOUND') {
-      throw pageNotFoundError(page)
-    }
-    console.error(err)
-    // If this is not a MODULE_NOT_FOUND error,
-    // it should be something with the content of the page.
-    // So, Next.js rendering system will catch it and process.
-    throw err
+export default async function requirePage (page, {dir, dist}) {
+  const pagePath = getPagePath(page, {dir, dist}) + '.js'
+  const fileExists = await fs.exists(pagePath)
+  if (!fileExists) {
+    throw pageNotFoundError(page)
   }
+
+  return require(pagePath)
 }
diff --git a/test/isolated/_resolvedata/dist/bundles/pages/non-existent-child.js b/test/isolated/_resolvedata/dist/bundles/pages/non-existent-child.js
new file mode 100644
index 00000000..806b4981
--- /dev/null
+++ b/test/isolated/_resolvedata/dist/bundles/pages/non-existent-child.js
@@ -0,0 +1,4 @@
+const nonExistent = require('./non-existent-module')
+module.exports = {
+  test: nonExistent
+}
diff --git a/test/isolated/require-page.test.js b/test/isolated/require-page.test.js
index e7e6e513..c3c0c262 100644
--- a/test/isolated/require-page.test.js
+++ b/test/isolated/require-page.test.js
@@ -57,26 +57,42 @@ describe('getPagePath', () => {
 })
 
 describe('requirePage', () => {
-  it('Should require /index.js when using /', () => {
-    const page = requirePage('/', {dir: __dirname, dist: '_resolvedata'})
+  it('Should require /index.js when using /', async () => {
+    const page = await 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'})
+  it('Should require /index.js when using /index', async () => {
+    const page = await 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'})
+  it('Should require /world.js when using /world', async () => {
+    const page = await 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 /../../test.js', async () => {
+    try {
+      await requirePage('/../../test', {dir: __dirname, dist: '_resolvedata'})
+    } catch (err) {
+      expect(err.code).toBe('ENOENT')
+    }
   })
 
-  it('Should throw when using non existent pages like /non-existent.js', () => {
-    expect(() => requirePage('/non-existent.js', {dir: __dirname, dist: '_resolvedata'})).toThrow()
+  it('Should throw when using non existent pages like /non-existent.js', async () => {
+    try {
+      await requirePage('/non-existent', {dir: __dirname, dist: '_resolvedata'})
+    } catch (err) {
+      expect(err.code).toBe('ENOENT')
+    }
+  })
+
+  it('Should bubble up errors in the child component', async () => {
+    try {
+      await requirePage('/non-existent-child', {dir: __dirname, dist: '_resolvedata'})
+    } catch (err) {
+      expect(err.code).toBe('MODULE_NOT_FOUND')
+    }
   })
 })