Add required permanent: boolean field to redirect (#10044)

* Add permanent field for redirects * Update printing redirect status code * Don't add permanent to routes-manifest Co-authored-by: Joe Haddad <timer150@gmail.com>
vercel · Jan 14, 2020 · 84264f8 · 84264f8
1 parent c30e542
commit 84264f8
Show file tree

Hide file tree

Showing 8 changed files with 51 additions and 18 deletions.
diff --git a/packages/next/build/index.ts b/packages/next/build/index.ts
@@ -10,6 +10,7 @@ import { pathToRegexp } from 'path-to-regexp'
 import { promisify } from 'util'
 import formatWebpackMessages from '../client/dev/error-overlay/format-webpack-messages'
 import checkCustomRoutes, {
+  getRedirectStatus,
   RouteType,
   Redirect,
   Rewrite,
@@ -22,7 +23,6 @@ import { recursiveReadDir } from '../lib/recursive-readdir'
 import { verifyTypeScriptSetup } from '../lib/verifyTypeScriptSetup'
 import {
   BUILD_MANIFEST,
-  DEFAULT_REDIRECT_STATUS,
   EXPORT_DETAIL,
   EXPORT_MARKER,
   PAGES_MANIFEST,
@@ -246,7 +246,8 @@ export default async function build(dir: string, conf = null): Promise<void> {
       ...r,
       ...(type === 'redirect'
         ? {
-            statusCode: r.statusCode || DEFAULT_REDIRECT_STATUS,
+            statusCode: getRedirectStatus(r as Redirect),
+            permanent: undefined,
           }
         : {}),
       regex: routeRegex.source,

diff --git a/packages/next/build/utils.ts b/packages/next/build/utils.ts
@@ -4,11 +4,14 @@ import textTable from 'next/dist/compiled/text-table'
 import path from 'path'
 import { isValidElementType } from 'react-is'
 import stripAnsi from 'strip-ansi'
-import { Redirect, Rewrite } from '../lib/check-custom-routes'
+import {
+  Redirect,
+  Rewrite,
+  getRedirectStatus,
+} from '../lib/check-custom-routes'
 import { SPR_GET_INITIAL_PROPS_CONFLICT } from '../lib/constants'
 import prettyBytes from '../lib/pretty-bytes'
 import { recursiveReadDir } from '../lib/recursive-readdir'
-import { DEFAULT_REDIRECT_STATUS } from '../next-server/lib/constants'
 import { getRouteMatcher, getRouteRegex } from '../next-server/lib/router/utils'
 import { isDynamicRoute } from '../next-server/lib/router/utils/is-dynamic'
 import { findPageFile } from '../server/lib/find-page-file'
@@ -250,10 +253,7 @@ export function printCustomRoutes({
               route.source,
               route.destination,
               ...(isRedirects
-                ? [
-                    ((route as Redirect).statusCode ||
-                      DEFAULT_REDIRECT_STATUS) + '',
-                  ]
+                ? [getRedirectStatus(route as Redirect) + '']
                 : []),
             ]
           }),

diff --git a/packages/next/lib/check-custom-routes.ts b/packages/next/lib/check-custom-routes.ts
@@ -1,4 +1,8 @@
 import { match as regexpMatch } from 'path-to-regexp'
+import {
+  PERMANENT_REDIRECT_STATUS,
+  TEMPORARY_REDIRECT_STATUS,
+} from '../next-server/lib/constants'
 
 export type Rewrite = {
   source: string
@@ -7,6 +11,7 @@ export type Rewrite = {
 
 export type Redirect = Rewrite & {
   statusCode?: number
+  permanent?: boolean
 }
 
 export type Header = {
@@ -16,6 +21,13 @@ export type Header = {
 
 const allowedStatusCodes = new Set([301, 302, 303, 307, 308])
 
+export function getRedirectStatus(route: Redirect) {
+  return (
+    route.statusCode ||
+    (route.permanent ? PERMANENT_REDIRECT_STATUS : TEMPORARY_REDIRECT_STATUS)
+  )
+}
+
 function checkRedirect(route: Redirect) {
   const invalidParts: string[] = []
   let hadInvalidStatus: boolean = false
@@ -24,6 +36,10 @@ function checkRedirect(route: Redirect) {
     hadInvalidStatus = true
     invalidParts.push(`\`statusCode\` is not undefined or valid statusCode`)
   }
+  if (typeof route.permanent !== 'boolean' && !route.statusCode) {
+    invalidParts.push(`\`permanent\` is not set to \`true\` or \`false\``)
+  }
+
   return {
     invalidParts,
     hadInvalidStatus,
@@ -72,7 +88,7 @@ export default function checkCustomRoutes(
     allowedKeys = new Set([
       'source',
       'destination',
-      ...(isRedirect ? ['statusCode'] : []),
+      ...(isRedirect ? ['statusCode', 'permanent'] : []),
     ])
   } else {
     allowedKeys = new Set(['source', 'headers'])
@@ -106,7 +122,7 @@ export default function checkCustomRoutes(
 
     if (type === 'redirect') {
       const result = checkRedirect(route as Redirect)
-      hadInvalidStatus = result.hadInvalidStatus
+      hadInvalidStatus = hadInvalidStatus || result.hadInvalidStatus
       invalidParts.push(...result.invalidParts)
     }
 

diff --git a/packages/next/next-server/lib/constants.ts b/packages/next/next-server/lib/constants.ts
@@ -32,4 +32,5 @@ export const IS_BUNDLED_PAGE_REGEX = /^static[/\\][^/\\]+[/\\]pages.*\.js$/
 // matches static/<buildid>/pages/:page*.js
 export const ROUTE_NAME_REGEX = /^static[/\\][^/\\]+[/\\]pages[/\\](.*)\.js$/
 export const SERVERLESS_ROUTE_NAME_REGEX = /^pages[/\\](.*)\.js$/
-export const DEFAULT_REDIRECT_STATUS = 307
+export const TEMPORARY_REDIRECT_STATUS = 307
+export const PERMANENT_REDIRECT_STATUS = 308
diff --git a/packages/next/next-server/server/next-server.ts b/packages/next/next-server/server/next-server.ts
@@ -17,7 +17,6 @@ import {
   ROUTES_MANIFEST,
   SERVER_DIRECTORY,
   SERVERLESS_DIRECTORY,
-  DEFAULT_REDIRECT_STATUS,
 } from '../lib/constants'
 import {
   getRouteMatcher,
@@ -50,6 +49,7 @@ import {
   Rewrite,
   RouteType,
   Header,
+  getRedirectStatus,
 } from '../../lib/check-custom-routes'
 
 const getCustomRouteMatcher = pathMatch(true)
@@ -493,8 +493,7 @@ export default class Server {
                 })
 
                 res.setHeader('Location', updatedDestination)
-                res.statusCode =
-                  (route as Redirect).statusCode || DEFAULT_REDIRECT_STATUS
+                res.statusCode = getRedirectStatus(route as Redirect)
 
                 // Since IE11 doesn't support the 308 header add backwards
                 // compatibility using refresh header

diff --git a/test/integration/custom-routes/next.config.js b/test/integration/custom-routes/next.config.js
@@ -77,10 +77,12 @@ module.exports = {
         {
           source: '/hello/:id/another',
           destination: '/blog/:id',
+          permanent: false,
         },
         {
           source: '/redirect1',
           destination: '/',
+          permanent: false,
         },
         {
           source: '/redirect2',
@@ -95,7 +97,7 @@ module.exports = {
         {
           source: '/redirect4',
           destination: '/',
-          statusCode: 308,
+          permanent: true,
         },
         {
           source: '/redir-chain1',
@@ -115,10 +117,12 @@ module.exports = {
         {
           source: '/to-external',
           destination: 'https://google.com',
+          permanent: false,
         },
         {
           source: '/query-redirect/:section/:name',
           destination: '/with-params?first=:section&second=:name',
+          permanent: false,
         },
       ]
     },

diff --git a/test/integration/custom-routes/test/index.test.js b/test/integration/custom-routes/test/index.test.js
@@ -65,7 +65,7 @@ const runTests = (isDev = false) => {
     expect(res3location).toBe('/')
   })
 
-  it('should redirect successfully with default statusCode', async () => {
+  it('should redirect successfully with permanent: false', async () => {
     const res = await fetchViaHTTP(appPort, '/redirect1', undefined, {
       redirect: 'manual',
     })

diff --git a/test/integration/invalid-custom-routes/test/index.test.js b/test/integration/invalid-custom-routes/test/index.test.js
@@ -28,11 +28,13 @@ const invalidRedirects = [
   {
     // missing destination
     source: '/hello',
+    permanent: false,
   },
   {
     // invalid source
     source: 123,
     destination: '/another',
+    permanent: false,
   },
   {
     // invalid statusCode type
@@ -46,15 +48,21 @@ const invalidRedirects = [
     destination: '/another',
     statusCode: 404,
   },
+  {
+    // invalid permanent value
+    source: '/hello',
+    destination: '/another',
+    permanent: 'yes',
+  },
 ]
 
 const invalidRedirectAssertions = (stderr = '') => {
   expect(stderr).toContain(
-    `\`destination\` is missing for route {"source":"/hello"}`
+    `\`destination\` is missing for route {"source":"/hello","permanent":false}`
   )
 
   expect(stderr).toContain(
-    `\`source\` is not a string for route {"source":123,"destination":"/another"}`
+    `\`source\` is not a string for route {"source":123,"destination":"/another","permanent":false}`
   )
 
   expect(stderr).toContain(
@@ -65,6 +73,10 @@ const invalidRedirectAssertions = (stderr = '') => {
     `\`statusCode\` is not undefined or valid statusCode for route {"source":"/hello","destination":"/another","statusCode":404}`
   )
 
+  expect(stderr).toContain(
+    `\`permanent\` is not set to \`true\` or \`false\` for route {"source":"/hello","destination":"/another","permanent":"yes"}`
+  )
+
   expect(stderr).toContain(
     'Valid redirect statusCode values are 301, 302, 303, 307, 308'
   )