Remove special error script handling (#3849)

* Remove special error script handling.
As a result of that, we can't detect 500 errors and buildIdMismatch via client side.

* Fix failing test cases.

* Refactor the code base.

* Remove Router.onAppUpdated

errors/no-on-app-updated-hook.md

@@ -0,0 +1,25 @@
+# Router.onAppUpdated is removed
+
+Due to [this bug fix](https://github.com/zeit/next.js/pull/3849), we had to remove the `Router.onAppUpdated` hook. But the default functionality of this feature is still in effect.
+
+We use this hook to detect a new app deployment when switching pages and act accordingly. Although there are many things you can do in this hook, it's often used to navigate the page via the server as shown below:
+
+```js
+Router.onAppUpdated = function(nextRoute) {
+  location.href = nextRoute
+}
+```
+
+In this hook, you can't wait for a network request or a promise to get resolved. And you can't block the page navigation. So, the things you can do is limited.
+
+One real use of this hook is to persist your application state to local-storage before the page navigation. For that, you can use the [`window.onbeforeunload`](https://developer.mozilla.org/en-US/docs/Web/API/WindowEventHandlers/onbeforeunload) hook instead.
+
+This is code for that:
+
+```js
+window.onbeforeunload = function(e) {
+  // Get the application state (usually from a store like Redux)
+  const appState = {}
+  localStorage.setItem('app-state', JSON.stringify(appState));
+};
+```

lib/page-loader.js

@@ -76,6 +76,7 @@ export default class PageLoader {
     script.src = url
     script.onerror = () => {
       const error = new Error(`Error when loading route: ${route}`)
+      error.code = 'PAGE_LOAD_ERROR'
       this.pageRegisterEvents.emit(route, { error })
     }
 

lib/router/index.js

@@ -1,5 +1,6 @@
 /* global window */
 import _Router from './router'
+import { execOnce } from '../utils'
 
 const SingletonRouter = {
   router: null, // holds the actual router instance
@@ -53,6 +54,18 @@ routerEvents.forEach((event) => {
   })
 })
 
+const warnAboutRouterOnAppUpdated = execOnce(() => {
+  console.warn(`Router.onAppUpdated is removed - visit https://err.sh/next.js/no-on-app-updated-hook for more information.`)
+})
+
+Object.defineProperty(SingletonRouter, 'onAppUpdated', {
+  get () { return null },
+  set () {
+    warnAboutRouterOnAppUpdated()
+    return null
+  }
+})
+
 function throwIfNoRouter () {
   if (!SingletonRouter.router) {
     const message = 'No router instance found.\n' +
@@ -85,15 +98,6 @@ export const createRouter = function (...args) {
 // Export the actual Router class, which is usually used inside the server
 export const Router = _Router
 
-export function _notifyBuildIdMismatch (nextRoute) {
-  if (SingletonRouter.onAppUpdated) {
-    SingletonRouter.onAppUpdated(nextRoute)
-  } else {
-    console.warn(`An app update detected. Loading the SSR version of "${nextRoute}"`)
-    window.location.href = nextRoute
-  }
-}
-
 export function _rewriteUrlForNextExport (url) {
   const [, hash] = url.split('#')
   url = url.replace(/#.*/, '')

lib/router/router.js

@@ -5,7 +5,7 @@ import EventEmitter from '../EventEmitter'
 import shallowEquals from '../shallow-equals'
 import PQueue from '../p-queue'
 import { loadGetInitialProps, getURL, warn, execOnce } from '../utils'
-import { _notifyBuildIdMismatch, _rewriteUrlForNextExport } from './'
+import { _rewriteUrlForNextExport } from './'
 
 export default class Router {
   constructor (pathname, query, as, { pageLoader, Component, ErrorComponent, err } = {}) {
@@ -212,20 +212,12 @@ export default class Router {
 
       this.components[route] = routeInfo
     } catch (err) {
-      if (err.buildIdMismatched) {
+      if (err.code === 'PAGE_LOAD_ERROR') {
-        // Now we need to reload the page or do the action asked by the user
+        // If we can't load the page it could be one of following reasons
-        _notifyBuildIdMismatch(as)
+        //  1. Page doesn't exists
-        // We also need to cancel this current route change.
+        //  2. Page does exist in a different zone
-        // We do it like this.
+        //  3. Internal error while loading the page
-        err.cancelled = true
-        return { error: err }
-      }
 
-      if (err.statusCode === 404) {
-        // If there's 404 error for the page, it could be due to two reasons.
-        //  1. Page is not exists
-        //  2. Page is exists in a different zone
-        // We are not sure whether this is actual 404 or exists in a different zone.
         // So, doing a hard reload is the proper way to deal with this.
         window.location.href = as
 

package.json

@@ -106,8 +106,7 @@
     "webpack-dev-middleware": "1.12.0",
     "webpack-hot-middleware": "2.21.0",
     "webpack-sources": "1.1.0",
-    "write-file-webpack-plugin": "4.2.0",
+    "write-file-webpack-plugin": "4.2.0"
-    "xss-filters": "1.2.7"
   },
   "devDependencies": {
     "@taskr/babel": "1.1.0",

readme.md

@@ -491,7 +491,6 @@ Here's a list of supported events:
 - `onRouteChangeComplete(url)` - Fires when a route changed completely
 - `onRouteChangeError(err, url)` - Fires when there's an error when changing routes
 - `onBeforeHistoryChange(url)` - Fires just before changing the browser's history
-- `onAppUpdated(nextRoute)` - Fires when switching pages and there's a new version of the app
 
 > Here `url` is the URL shown in the browser. If you call `Router.push(url, as)` (or similar), then the value of `url` will be `as`.
 
@@ -519,17 +518,6 @@ Router.onRouteChangeError = (err, url) => {
 }
 ```
 
-If you change a route while in between a new deployment, we can't navigate the app via client side. We need to do a full browser navigation. We do it automatically for you.
-
-But you can customize that via `Route.onAppUpdated` event like this:
-
-```js
-Router.onAppUpdated = nextUrl => {
-  // persist the local state
-  location.href = nextUrl
-}
-```
-
 ##### Shallow Routing
 
 <p><details>

server/index.js

@@ -196,9 +196,7 @@ export default class Server {
       '/_next/:buildId/page/_error.js': async (req, res, params) => {
         if (!this.handleBuildId(params.buildId, res)) {
           const error = new Error('INVALID_BUILD_ID')
-          const customFields = { buildIdMismatched: true }
+          return await renderScriptError(req, res, '/_error', error)
-
-          return await renderScriptError(req, res, '/_error', error, customFields, this.renderOpts)
         }
 
         const p = join(this.dir, `${this.dist}/bundles/pages/_error.js`)
@@ -211,22 +209,19 @@ export default class Server {
 
         if (!this.handleBuildId(params.buildId, res)) {
           const error = new Error('INVALID_BUILD_ID')
-          const customFields = { buildIdMismatched: true }
+          return await renderScriptError(req, res, page, error)
-
-          return await renderScriptError(req, res, page, error, customFields, this.renderOpts)
         }
 
         if (this.dev) {
           try {
             await this.hotReloader.ensurePage(page)
           } catch (error) {
-            return await renderScriptError(req, res, page, error, {}, this.renderOpts)
+            return await renderScriptError(req, res, page, error)
           }
 
           const compilationErr = await this.getCompilationError()
           if (compilationErr) {
-            const customFields = { statusCode: 500 }
+            return await renderScriptError(req, res, page, compilationErr)
-            return await renderScriptError(req, res, page, compilationErr, customFields, this.renderOpts)
           }
         }
 
@@ -235,7 +230,7 @@ export default class Server {
         // [production] If the page is not exists, we need to send a proper Next.js style 404
         // Otherwise, it'll affect the multi-zones feature.
         if (!(await fsAsync.exists(p))) {
-          return await renderScriptError(req, res, page, { code: 'ENOENT' }, {}, this.renderOpts)
+          return await renderScriptError(req, res, page, { code: 'ENOENT' })
         }
 
         await this.serveStatic(req, res, p)

server/render.js

@@ -12,7 +12,8 @@ import Head, { defaultHead } from '../lib/head'
 import App from '../lib/app'
 import ErrorDebug from '../lib/error-debug'
 import { flushChunks } from '../lib/dynamic'
-import xssFilters from 'xss-filters'
+
+const logger = console
 
 export async function render (req, res, pathname, query, opts) {
   const html = await renderToHTML(req, res, pathname, query, opts)
@@ -120,36 +121,19 @@ async function doRender (req, res, pathname, query, {
   return '<!DOCTYPE html>' + renderToStaticMarkup(doc)
 }
 
-export async function renderScriptError (req, res, page, error, customFields, { dev }) {
+export async function renderScriptError (req, res, page, error) {
   // Asks CDNs and others to not to cache the errored page
   res.setHeader('Cache-Control', 'no-store, must-revalidate')
-  // prevent XSS attacks by filtering the page before printing it.
-  page = xssFilters.uriInSingleQuotedAttr(page)
-  res.setHeader('Content-Type', 'text/javascript')
 
   if (error.code === 'ENOENT') {
-    res.end(`
+    res.statusCode = 404
-      window.__NEXT_REGISTER_PAGE('${page}', function() {
+    res.end('404 - Not Found')
-        var error = new Error('Page does not exist: ${page}')
-        error.statusCode = 404
-
-        return { error: error }
-      })
-    `)
     return
   }
 
-  const errorJson = {
+  logger.error(error.stack)
-    ...serializeError(dev, error),
+  res.statusCode = 500
-    ...customFields
+  res.end('500 - Internal Error')
-  }
-
-  res.end(`
-    window.__NEXT_REGISTER_PAGE('${page}', function() {
-      var error = ${JSON.stringify(errorJson)}
-      return { error: error }
-    })
-  `)
 }
 
 export function sendHTML (req, res, html, method, { dev }) {

test/integration/production/test/index.test.js

@@ -127,7 +127,7 @@ describe('Production Usage', () => {
       // Let the browser to prefetch the page and error it on the console.
       await waitFor(3000)
       const browserLogs = await browser.log('browser')
-      expect(browserLogs[0].message).toMatch(/Page does not exist: \/no-such-page/)
+      expect(browserLogs[0].message).toMatch(/\/no-such-page.js - Failed to load resource/)
 
       // When we go to the 404 page, it'll do a hard reload.
       // So, it's possible for the front proxy to load a page from another zone.