Match public files priority in dev (vercel#8641)

* Match public files priority in dev

* Remove un-needed old public file handling

* Run public file priority test for non-dev also

* Make sure to enable publicDirectory

* Add checking for conflicting pages in dev and during build

* Add error for public dir middleware conflict

* Add err.sh for _next conflict

* Move up public dir conflict checking

* Only run _next conflict check in dev when path contains it

errors/conflicting-public-file-page.md

@@ -0,0 +1,29 @@
+# Conflicting Public File and Page File
+
+#### Why This Error Occurred
+
+One of your public files has the exact same path as a page file which is not supported. Since only one resource can reside at the URL both public files and page files must be unique.
+
+#### Possible Ways to Fix It
+
+Rename either the public file or page file that is causing the conflict.
+
+Example conflict between public file and page file
+```
+public/
+  hello
+pages/
+  hello.js
+```
+
+Non-conflicting public file and page file
+```
+public/
+  hello.txt
+pages/
+  hello.js
+```
+
+### Useful Links
+
+- [Static file serving docs](https://nextjs.org/docs#static-file-serving-eg-images)

errors/public-next-folder-conflict.md

@@ -0,0 +1,9 @@
+# Public \_next folder Conflict
+
+#### Why This Error Occurred
+
+In your `./public` folder you added a `_next` folder which conflicts with the internal usage of `_next`. Due to this conflicting this folder name is not allowed inside of your `public` folder.
+
+#### Possible Ways to Fix It
+
+Rename the `_next` folder in your `public` folder to something else or remove it.

packages/next/build/index.ts

@@ -18,6 +18,7 @@ import Worker from 'jest-worker'
 
 import formatWebpackMessages from '../client/dev/error-overlay/format-webpack-messages'
 import { recursiveDelete } from '../lib/recursive-delete'
+import { recursiveReadDir } from '../lib/recursive-readdir'
 import { verifyTypeScriptSetup } from '../lib/verifyTypeScriptSetup'
 import {
   recordBuildDuration,
@@ -40,9 +41,11 @@ import getBaseWebpackConfig from './webpack-config'
 import { getPageChunks } from './webpack/plugins/chunk-graph-plugin'
 import { writeBuildId } from './write-build-id'
 import createSpinner from './spinner'
+import { PUBLIC_DIR_MIDDLEWARE_CONFLICT } from '../lib/constants'
 
 const fsUnlink = promisify(fs.unlink)
 const fsRmdir = promisify(fs.rmdir)
+const fsStat = promisify(fs.stat)
 const fsMove = promisify(fs.rename)
 const fsReadFile = promisify(fs.readFile)
 const fsWriteFile = promisify(fs.writeFile)
@@ -79,6 +82,12 @@ export default async function build(dir: string, conf = null): Promise<void> {
   const buildId = await generateBuildId(config.generateBuildId, nanoid)
   const distDir = path.join(dir, config.distDir)
   const pagesDir = path.join(dir, 'pages')
+  const publicDir = path.join(dir, 'public')
+  let publicFiles: string[] = []
+
+  if (config.experimental.publicDirectory) {
+    publicFiles = await recursiveReadDir(publicDir, /.*/)
+  }
 
   let tracer: any = null
   if (config.experimental.profiling) {
@@ -101,6 +110,36 @@ export default async function build(dir: string, conf = null): Promise<void> {
 
   const mappedPages = createPagesMapping(pagePaths, config.pageExtensions)
   const entrypoints = createEntrypoints(mappedPages, target, buildId, config)
+  const conflictingPublicFiles: string[] = []
+
+  try {
+    await fsStat(path.join(publicDir, '_next'))
+    throw new Error(PUBLIC_DIR_MIDDLEWARE_CONFLICT)
+  } catch (err) {}
+
+  for (let file of publicFiles) {
+    file = file
+      .replace(/\\/g, '/')
+      .replace(/\/index$/, '')
+      .split(publicDir)
+      .pop()!
+
+    if (mappedPages[file]) {
+      conflictingPublicFiles.push(file)
+    }
+  }
+  const numConflicting = conflictingPublicFiles.length
+
+  if (numConflicting) {
+    throw new Error(
+      `Conflicting public and page file${
+        numConflicting === 1 ? ' was' : 's were'
+      } found. https://err.sh/zeit/next.js/conflicting-public-file-page\n${conflictingPublicFiles.join(
+        '\n'
+      )}`
+    )
+  }
+
   const configs = await Promise.all([
     getBaseWebpackConfig(dir, {
       tracer,

packages/next/lib/constants.ts

@@ -22,3 +22,5 @@ export const API_ROUTE = /^\/api(?:\/|$)/
 // we have to use a private alias
 export const PAGES_DIR_ALIAS = 'private-next-pages'
 export const DOT_NEXT_ALIAS = 'private-dot-next'
+
+export const PUBLIC_DIR_MIDDLEWARE_CONFLICT = `You can not have a '_next' folder inside of your public folder. This conflicts with the internal '/_next' route. https://err.sh/zeit/next.js/public-next-folder-conflict`

packages/next/server/next-dev-server.js

@@ -1,5 +1,6 @@
 import Server from '../next-server/server/next-server'
 import { join, relative } from 'path'
+import { promisify } from 'util'
 import HotReloader from './hot-reloader'
 import { route } from '../next-server/server/router'
 import { PHASE_DEVELOPMENT_SERVER } from '../next-server/lib/constants'
@@ -18,13 +19,18 @@ import {
   isDynamicRoute
 } from '../next-server/lib/router/utils'
 import React from 'react'
+import { findPageFile } from './lib/find-page-file'
+import { normalizePagePath } from '../next-server/server/normalize-page-path'
+import { PUBLIC_DIR_MIDDLEWARE_CONFLICT } from '../lib/constants'
 
 if (typeof React.Suspense === 'undefined') {
   throw new Error(
     `The version of React you are using is lower than the minimum required version needed for Next.js. Please upgrade "react" and "react-dom": "npm install --save react react-dom" https://err.sh/zeit/next.js/invalid-react-version`
   )
 }
 
+const fsStat = promisify(fs.stat)
+
 export default class DevServer extends Server {
   constructor (options) {
     super(options)
@@ -45,6 +51,7 @@ export default class DevServer extends Server {
         )
       })
     }
+    this.pagesDir = join(this.dir, 'pages')
   }
 
   currentPhase () {
@@ -191,6 +198,37 @@ export default class DevServer extends Server {
 
   async run (req, res, parsedUrl) {
     await this.devReady
+    const { pathname } = parsedUrl
+
+    if (pathname.startsWith('/_next')) {
+      try {
+        await fsStat(join(this.publicDir, '_next'))
+        throw new Error(PUBLIC_DIR_MIDDLEWARE_CONFLICT)
+      } catch (err) {}
+    }
+
+    // check for a public file, throwing error if there's a
+    // conflicting page
+    if (this.nextConfig.experimental.publicDirectory) {
+      if (await this.hasPublicFile(pathname)) {
+        const pageFile = await findPageFile(
+          this.pagesDir,
+
+          normalizePagePath(pathname),
+          this.nextConfig.pageExtensions
+        )
+
+        if (pageFile) {
+          const err = new Error(
+            `A conflicting public file and page file was found for path ${pathname} https://err.sh/zeit/next.js/conflicting-public-file-page`
+          )
+          res.statusCode = 500
+          return this.renderError(err, req, res, pathname, {})
+        }
+        return this.servePublic(req, res, pathname)
+      }
+    }
+
     const { finished } = await this.hotReloader.run(req, res, parsedUrl)
     if (finished) {
       return
@@ -272,9 +310,9 @@ export default class DevServer extends Server {
 
     // In dev mode we use on demand entries to compile the page before rendering
     try {
-      await this.hotReloader.ensurePage(pathname).catch(err => {
+      await this.hotReloader.ensurePage(pathname).catch(async err => {
         if (err.code !== 'ENOENT') {
-          return Promise.reject(err)
+          throw err
         }
 
         for (const dynamicRoute of this.dynamicRoutes) {
@@ -289,18 +327,12 @@ export default class DevServer extends Server {
           })
         }
 
-        return Promise.reject(err)
+        throw err
       })
     } catch (err) {
       if (err.code === 'ENOENT') {
-        if (this.nextConfig.experimental.publicDirectory) {
-          // Try to send a public file and let servePublic handle the request from here
-          await this.servePublic(req, res, pathname)
-          return null
-        } else {
-          res.statusCode = 404
-          return this.renderErrorToHTML(null, req, res, pathname, query)
-        }
+        res.statusCode = 404
+        return this.renderErrorToHTML(null, req, res, pathname, query)
       }
       if (!this.quiet) console.error(err)
     }
@@ -349,6 +381,15 @@ export default class DevServer extends Server {
     return this.serveStatic(req, res, p)
   }
 
+  async hasPublicFile (path) {
+    try {
+      const info = await fsStat(join(this.publicDir, path))
+      return info.isFile()
+    } catch (_) {
+      return false
+    }
+  }
+
   async getCompilationError (page) {
     const errors = await this.hotReloader.getCompilationErrors(page)
     if (errors.length === 0) return

test/integration/basic/test/public-folder.js

@@ -7,13 +7,5 @@ export default context => {
       const data = await renderViaHTTP(context.appPort, '/data/data.txt')
       expect(data).toBe('data')
     })
-
-    it('should prioritize pages over public files', async () => {
-      const html = await renderViaHTTP(context.appPort, '/about')
-      const data = await renderViaHTTP(context.appPort, '/file')
-
-      expect(html).toMatch(/About Page/)
-      expect(data).toBe('test')
-    })
   })
 }

test/integration/conflicting-public-file-page/next.config.js

@@ -0,0 +1,5 @@
+module.exports = {
+  experimental: {
+    publicDirectory: true
+  }
+}

test/integration/conflicting-public-file-page/pages/another/conflict.js

@@ -0,0 +1 @@
+export default () => `oops this is doesn't work`

test/integration/conflicting-public-file-page/pages/another/index.js

@@ -0,0 +1 @@
+export default () => `oops this is doesn't work`

test/integration/conflicting-public-file-page/pages/hello.js

@@ -0,0 +1 @@
+export default () => `oops this is doesn't work`

test/integration/conflicting-public-file-page/public/another/conflict

@@ -0,0 +1 @@
+oops this is doesn't work

test/integration/conflicting-public-file-page/public/another/index

@@ -0,0 +1 @@
+oops this is doesn't work

test/integration/conflicting-public-file-page/public/hello

@@ -0,0 +1 @@
+oops this is doesn't work

test/integration/conflicting-public-file-page/public/normal.txt

@@ -0,0 +1 @@
+no conflict here

test/integration/conflicting-public-file-page/test/index.test.js

@@ -0,0 +1,42 @@
+/* eslint-env jest */
+/* global jasmine */
+import path from 'path'
+import {
+  nextBuild,
+  launchApp,
+  findPort,
+  killApp,
+  renderViaHTTP,
+  waitFor
+} from 'next-test-utils'
+
+jasmine.DEFAULT_TIMEOUT_INTERVAL = 1000 * 60 * 5
+const appDir = path.join(__dirname, '..')
+
+describe('Errors on conflict between public file and page file', () => {
+  it('Throws error during development', async () => {
+    const appPort = await findPort()
+    const app = await launchApp(appDir, appPort)
+    const conflicts = ['/another/conflict', '/another/index', '/hello']
+
+    for (const conflict of conflicts) {
+      const html = await renderViaHTTP(appPort, conflict)
+      expect(html).toMatch(
+        /A conflicting public file and page file was found for path/
+      )
+    }
+    await waitFor(1000)
+    await killApp(app)
+  })
+
+  it('Throws error during build', async () => {
+    const conflicts = ['/another/conflict', '/another', '/hello']
+    const results = await nextBuild(appDir, [], { stdout: true, stderr: true })
+    const output = results.stdout + results.stderr
+    expect(output).toMatch(/Conflicting public and page files were found/)
+
+    for (const conflict of conflicts) {
+      expect(output.indexOf(conflict) > 0).toBe(true)
+    }
+  })
+})

test/integration/dynamic-routing/next.config.js

@@ -1,5 +1,6 @@
 module.exports = {
   experimental: {
-    modern: true
+    modern: true,
+    publicDirectory: true
   }
 }

test/integration/dynamic-routing/public/hello.txt

@@ -0,0 +1 @@
+hello world

test/integration/dynamic-routing/test/index.test.js

@@ -187,6 +187,11 @@ function runTests (dev) {
     expect(scrollPosition).toBe(7232)
   })
 
+  it('should prioritize public files over dynamic route', async () => {
+    const data = await renderViaHTTP(appPort, '/hello.txt')
+    expect(data).toMatch(/hello world/)
+  })
+
   if (dev) {
     it('should work with HMR correctly', async () => {
       const browser = await webdriver(appPort, '/post-1/comments')
@@ -245,7 +250,10 @@ describe('Dynamic Routing', () => {
           nextConfig,
           `
           module.exports = {
-            experimental: { modern: true }
+            experimental: {
+              modern: true,
+              publicDirectory: true
+            }
           }
         `
         )
@@ -269,7 +277,8 @@ describe('Dynamic Routing', () => {
         module.exports = {
           target: 'serverless',
           experimental: {
-            modern: true
+            modern: true,
+            publicDirectory: true
           }
         }
       `

test/integration/export/test/ssr.js

@@ -70,12 +70,10 @@ export default function (context) {
       expect(data).toBe('item')
     })
 
-    it('Should serve public files and prioritize pages', async () => {
+    it('Should serve public files', async () => {
       const html = await renderViaHTTP(context.port, '/about')
-      const html2 = await renderViaHTTP(context.port, '/query')
       const data = await renderViaHTTP(context.port, '/about/data.txt')
       expect(html).toMatch(/This is the About page foobar/)
-      expect(html2).toMatch(/{"a":"blue"}/)
       expect(data).toBe('data')
     })