Merge pull request #6 from Palmabit-IT/develop

Render Error Page if middleware has error

package.json

@@ -46,6 +46,7 @@
     "babel-polyfill": "^6.26.0",
     "eslint": "^5.5.0",
     "eslint-plugin-jest": "^21.22.0",
+    "lodash.clonedeep": "^4.5.0",
     "lodash.pick": "^4.4.0",
     "path-to-regexp": "^1.7.0",
     "prop-types": "^15.6.1",

src/Routes.js

@@ -3,7 +3,7 @@ import pick from 'lodash.pick'
 import NextLink from 'next/link'
 import NextRouter from 'next/router'
 import Route from './Route'
-import { generateRouteFromObjectName, redirectToLocalizedHome } from './helpers/routeHelper'
+import { generateRouteFromObjectName, redirectToLocalizedHome, detectLocale } from './helpers/routeHelper'
 import MiddlewareManager from './middleware/MiddlewareManager'
 
 export default class Routes {
@@ -49,7 +49,7 @@ export default class Routes {
     return this
   }
 
-  middleware(functions = []) {
+  middleware(functions) {
     if (!functions || !Array.isArray(functions)) {
       throw new Error('props must be an array')
     }
@@ -58,7 +58,7 @@ export default class Routes {
 
     functions.forEach((middleware, index) => {
       if (typeof middleware !== 'function') {
-        throw new Error(`middlewate at position ${index} is not a function`)
+        throw new Error(`middleware at position ${index} is not a function`)
       }
     })
 
@@ -71,9 +71,7 @@ export default class Routes {
   }
 
   findByName(name, locale = this.locale) {
-    if (name) {
-      return this.routes.filter(route => route.name === name && route.locale === locale)[0] || false
-    }
+    return name && this.routes.filter(route => route.name === name && route.locale === locale)[0] || false
   }
 
   match(url) {
@@ -94,8 +92,7 @@ export default class Routes {
   }
 
   findAndGetUrls(name, locale = this.locale, params = {}) {
-    const locl = locale || this.locale
-    const route = this.findByName(name, locl)
+    const route = this.findByName(name, locale)
 
     if (route) {
       return { route, urls: route.getUrls(params), byName: true }
@@ -128,18 +125,27 @@ export default class Routes {
         req.siteUrl = this.siteUrl
         req.getMultilanguageUrls = () => this.getMultilanguageUrls(route, query)
 
-        MiddlewareManager(route.middlewares, { req, res, route, query })((err, data) => {
-          if (err) throw err
+        const middleware = MiddlewareManager(route.middlewares, { req, res, route, query })
+        middleware((err, data) => {
+          if (err) {
+            const { pathname } = parsedUrl
+            const { statusCode = 500 } = err
+            res.statusCode = statusCode
+            app.renderError(err, req, res, pathname, query)
+            return
+          }
 
           req.nextData = data
+          renderRoute(app, customHandler, { req, res, route, query })
         })
 
-        renderRoute(app, customHandler, { req, res, route, query })
         return
       }
 
+
       if (req.url === '/' && this.forceLocale) {
-        redirectToLocalizedHome(res, this.locale)
+        const detectedLocale = detectLocale({ req, routes: this.routes, defaultLocale: this.locale })
+        redirectToLocalizedHome(res, detectedLocale)
         return
       }
 

src/helpers/routeHelper.js

@@ -24,4 +24,11 @@ export const redirectToLocalizedHome = (res, locale) => {
     res.writeHead(301, { 'Location': `/${locale}` })
     res.end()
   }
+}
+
+export const detectLocale = ({ req, routes, defaultLocale }) => {
+  const acceptsLangs = typeof req.acceptsLanguages === 'function' && req.acceptsLanguages()
+  const langs = !acceptsLangs ? [] : acceptsLangs.filter(lang => lang.length === 2)
+  const routesLangs = routes.map(({ locale }) => locale)
+  return routesLangs.indexOf(langs[0]) > -1 ? langs[0] : defaultLocale
 }

src/middleware/MiddlewareManager.js

@@ -1,10 +1,12 @@
 'use strict'
 
 import async from 'async'
+import clonedeep from 'lodash.clonedeep'
 
 export default (middlewares = [], initialData = {}) => {
+  const reversed = clonedeep(middlewares).reverse()
   return async.compose(
-    ...middlewares.reverse(),
+    ...reversed,
     function (cb) {
       cb(null, initialData)
     },

test/helpers/routeHelper.test.js

@@ -1,4 +1,4 @@
-import { generateRouteFromObjectName } from './../../src/helpers/routeHelper'
+import { generateRouteFromObjectName, detectLocale } from './../../src/helpers/routeHelper'
 
 describe('generateRouteFromObject()', () => {
   it('thrown err if name not exist', () => {
@@ -22,4 +22,26 @@ describe('generateRouteFromObject()', () => {
     expect(result).toHaveProperty('update')
     expect(result).not.toHaveProperty('foo')
   })
+
+  it('return detected locale', () => {
+    const req = {
+      acceptsLanguages: () => { return ['it-IT', 'it', 'en', 'es'] }
+    }
+
+    const routes = [{ locale: 'en' }, { locale: 'it' }]
+    const result = detectLocale({ req, routes, defaultLocale: 'en' })
+
+    expect(result).toBe('it')
+  })
+
+  it('return default locale if detected is non supoorted', () => {
+    const req = {
+      acceptsLanguages: () => { return ['it-IT', 'es'] }
+    }
+
+    const routes = [{ locale: 'en' }, { locale: 'it' }]
+    const result = detectLocale({ req, routes, defaultLocale: 'en' })
+
+    expect(result).toBe('en')
+  })
 })

test/routes.test.js

@@ -197,7 +197,7 @@ describe('findAndGetUrls', () => {
     expect(result.urls.as).toBe('/en/news')
   })
 
-  test('can thrown exception if route not exist', () => {
+  test('should throw an exception if route does not exist', () => {
     const routes = nextRoutes({ locale: 'it' })
     routes.add('news', 'en', '/news', 'newsList')
     routes.add('news', 'it', '/notizie', 'newsList')
@@ -206,6 +206,15 @@ describe('findAndGetUrls', () => {
       routes.findAndGetUrls('pdoor', 'fr')
     }).toThrow()
   })
+
+  test('should return path with locale if requested locale equals default and forceLocale is true', () => {
+    const routes = nextRoutes({ locale: 'it', forceLocale: true })
+    routes.add('news', 'it', '/notizie', 'newsList')
+
+    const { urls = {} } = routes.findAndGetUrls('news', 'it')
+    const { as = '' } = urls
+    expect(as.startsWith('/it')).toBeTrue()
+  })
 })
 
 describe('middleware()', () => {
@@ -227,4 +236,29 @@ describe('middleware()', () => {
     expect(routes.routes[0].middlewares).toBeInstanceOf(Array)
     expect(routes.routes[0].middlewares[0]).toBe(testFunct)
   })
+
+  test('can render error page if a middleware callback has error', () => {
+    const testFunct = (params, cb) => {
+      const error = new Error('this is an error')
+      error.statusCode = 418
+      cb(error)
+    }
+
+    const routes = nextRoutes({ locale: 'it' })
+    routes.add('a', 'en', '/').middleware([testFunct])
+
+    const app = {
+      getRequestHandler: () => { },
+      renderError: jest.fn()
+    }
+    const requestHandler = routes.getRequestHandler(app)
+    const req = { url: '/en' }
+    const res = {}
+
+    requestHandler(req, res)
+
+    expect(app.renderError.mock.calls.length).toBe(1)
+    expect(res.statusCode).toBe(418)
+
+  })
 })