Fix issue with afterFiles rewrites not working when prefixed with ass…

…etPrefix (vercel#68694) This PR fixes two issues with the use of `assetPrefix`: #1: vercel#64710 `assetPrefix` needs to be handled in `dev`, `deploy`, and `start`. In the current approach, only `dev` and `start` were handled, but a quirk of the implementation caused rewrites for non-asset paths to not be able to be used in `afterFiles` rewrites. vercel#2: When deploying Next.js (such as on Vercel), you need to add your own `beforeFiles` rewrite for `/${assetPrefix}/_next/...` requests or otherwise they would 404. This PR creates an automatically added `rewrite` to `beforeFiles` that handles the case for `dev`, `start`, and `deploy`, removes the existing logic in `filesystem.ts`, and adds more tests to check the behavior.
eps1lon · Aug 15, 2024 · d2cfaf4 · d2cfaf4
1 parent f87cf5c
commit d2cfaf4
Show file tree

Hide file tree

Showing 12 changed files with 289 additions and 11 deletions.
diff --git a/packages/next/src/lib/load-custom-routes.test.ts b/packages/next/src/lib/load-custom-routes.test.ts
@@ -0,0 +1,163 @@
+import loadCustomRoutes from './load-custom-routes'
+
+describe('loadCustomRoutes', () => {
+  describe('rewrites', () => {
+    it('missing rewrites should not throw', async () => {
+      const customRoutes = await loadCustomRoutes({})
+      expect(customRoutes.rewrites.beforeFiles).toEqual([])
+      expect(customRoutes.rewrites.afterFiles).toEqual([])
+      expect(customRoutes.rewrites.fallback).toEqual([])
+    })
+
+    it('array rewrites should be added to afterFiles', async () => {
+      const customRoutes = await loadCustomRoutes({
+        async rewrites() {
+          return [
+            {
+              source: '/a',
+              destination: '/b',
+            },
+          ]
+        },
+      })
+      expect(customRoutes.rewrites.beforeFiles).toEqual([])
+      expect(customRoutes.rewrites.afterFiles).toEqual([
+        {
+          destination: '/b',
+          source: '/a',
+        },
+      ])
+      expect(customRoutes.rewrites.fallback).toEqual([])
+    })
+
+    it('rewrites should be preserved correctly', async () => {
+      const customRoutes = await loadCustomRoutes({
+        async rewrites() {
+          return {
+            beforeFiles: [
+              {
+                source: '/beforeFiles/a',
+                destination: '/beforeFiles/b',
+              },
+            ],
+            afterFiles: [
+              {
+                source: '/afterFiles/a',
+                destination: '/afterFiles/b',
+              },
+            ],
+            fallback: [
+              {
+                source: '/fallback/a',
+                destination: '/fallback/b',
+              },
+            ],
+          }
+        },
+      })
+      expect(customRoutes.rewrites.beforeFiles).toEqual([
+        {
+          destination: '/beforeFiles/b',
+          source: '/beforeFiles/a',
+        },
+      ])
+      expect(customRoutes.rewrites.afterFiles).toEqual([
+        {
+          destination: '/afterFiles/b',
+          source: '/afterFiles/a',
+        },
+      ])
+      expect(customRoutes.rewrites.fallback).toEqual([
+        {
+          destination: '/fallback/b',
+          source: '/fallback/a',
+        },
+      ])
+    })
+
+    describe('assetPrefix', () => {
+      it('automatically inserts assetPrefix rewrite for /_next/ paths', async () => {
+        const customRoutes = await loadCustomRoutes({
+          assetPrefix: '/custom-asset-prefix',
+          async rewrites() {
+            return [
+              {
+                source: '/a',
+                destination: '/b',
+              },
+            ]
+          },
+        })
+        expect(customRoutes.rewrites.beforeFiles).toEqual([
+          {
+            destination: '/_next/:path+',
+            source: '/custom-asset-prefix/_next/:path+',
+          },
+        ])
+        expect(customRoutes.rewrites.afterFiles).toEqual([
+          {
+            destination: '/b',
+            source: '/a',
+          },
+        ])
+      })
+
+      it('automatically inserts assetPrefix rewrite for /_next/ paths when rewrites() is not present', async () => {
+        const customRoutes = await loadCustomRoutes({
+          assetPrefix: '/custom-asset-prefix',
+        })
+        expect(customRoutes.rewrites.beforeFiles).toEqual([
+          {
+            destination: '/_next/:path+',
+            source: '/custom-asset-prefix/_next/:path+',
+          },
+        ])
+        expect(customRoutes.rewrites.afterFiles).toEqual([])
+      })
+
+      it('automatically inserts assetPrefix rewrite for /_next/ paths for basePath', async () => {
+        const customRoutes = await loadCustomRoutes({
+          assetPrefix: '/custom-asset-prefix',
+          basePath: '/custom-base-path',
+          async rewrites() {
+            return [
+              {
+                source: '/a',
+                destination: '/b',
+              },
+            ]
+          },
+        })
+        expect(customRoutes.rewrites.beforeFiles).toEqual([
+          {
+            destination: '/custom-base-path/_next/:path+',
+            source: '/custom-asset-prefix/_next/:path+',
+          },
+        ])
+        expect(customRoutes.rewrites.afterFiles).toEqual([
+          {
+            destination: '/custom-base-path/b',
+            source: '/custom-base-path/a',
+          },
+        ])
+      })
+
+      it('does not insert assetPrefix rewrite for /_next/ paths when assetPrefix is absolute URL', async () => {
+        const customRoutes = await loadCustomRoutes({
+          assetPrefix: 'https://example.com/custom-asset-prefix',
+        })
+        expect(customRoutes.rewrites.beforeFiles).toEqual([])
+        expect(customRoutes.rewrites.afterFiles).toEqual([])
+      })
+
+      it('does not add rewrite when assetPrefix === basePath', async () => {
+        const customRoutes = await loadCustomRoutes({
+          assetPrefix: '/base',
+          basePath: '/base',
+        })
+        expect(customRoutes.rewrites.beforeFiles).toEqual([])
+        expect(customRoutes.rewrites.afterFiles).toEqual([])
+      })
+    })
+  })
+})
diff --git a/packages/next/src/lib/load-custom-routes.ts b/packages/next/src/lib/load-custom-routes.ts
@@ -5,6 +5,7 @@ import { bold, yellow } from './picocolors'
 import { escapeStringRegexp } from '../shared/lib/escape-regexp'
 import { tryToParsePath } from './try-to-parse-path'
 import { allowedStatusCodes } from './redirect-status'
+import { isFullStringUrl } from './url'
 
 export type RouteHas =
   | {
@@ -592,9 +593,28 @@ async function loadRedirects(config: NextConfig) {
 }
 
 async function loadRewrites(config: NextConfig) {
+  // If assetPrefix is set, add a rewrite for `/${assetPrefix}/_next/*`
+  // requests so that they are handled in any of dev, start, or deploy
+  // automatically without the user having to configure this.
+  // If the assetPrefix is an absolute URL, we can't add an automatic rewrite.
+  let maybeAssetPrefixRewrite: Rewrite[] = []
+  if (config.assetPrefix && !isFullStringUrl(config.assetPrefix)) {
+    const assetPrefix = config.assetPrefix.startsWith('/')
+      ? config.assetPrefix
+      : `/${config.assetPrefix}`
+    const basePath = config.basePath || ''
+    // If these are the same, then this would result in an infinite rewrite.
+    if (assetPrefix !== basePath) {
+      maybeAssetPrefixRewrite.push({
+        source: `${assetPrefix}/_next/:path+`,
+        destination: `${basePath}/_next/:path+`,
+      })
+    }
+  }
+
   if (typeof config.rewrites !== 'function') {
     return {
-      beforeFiles: [],
+      beforeFiles: [...maybeAssetPrefixRewrite],
       afterFiles: [],
       fallback: [],
     }
@@ -631,7 +651,10 @@ async function loadRewrites(config: NextConfig) {
     fallback: fallback.map((r) => ({ ...r })),
   }
 
-  beforeFiles = processRoutes(beforeFiles, config, 'rewrite')
+  beforeFiles = [
+    ...maybeAssetPrefixRewrite,
+    ...processRoutes(beforeFiles, config, 'rewrite'),
+  ]
   afterFiles = processRoutes(afterFiles, config, 'rewrite')
   fallback = processRoutes(fallback, config, 'rewrite')
 

diff --git a/packages/next/src/server/lib/router-utils/filesystem.ts b/packages/next/src/server/lib/router-utils/filesystem.ts
@@ -421,21 +421,18 @@ export async function setupFsCheck(opts: {
         return lruResult
       }
 
-      const { basePath, assetPrefix } = opts.config
+      const { basePath } = opts.config
 
       const hasBasePath = pathHasPrefix(itemPath, basePath)
-      const hasAssetPrefix = pathHasPrefix(itemPath, assetPrefix)
 
-      // Return null if either path doesn't start with basePath or assetPrefix
-      if ((basePath || assetPrefix) && !hasBasePath && !hasAssetPrefix) {
+      // Return null if path doesn't start with basePath
+      if (basePath && !hasBasePath) {
         return null
       }
 
-      // Either remove basePath or assetPrefix, not both (due to routes with same name as basePath)
+      // Remove basePath if it exists.
       if (basePath && hasBasePath) {
         itemPath = removePathPrefix(itemPath, basePath) || '/'
-      } else if (assetPrefix && hasAssetPrefix) {
-        itemPath = removePathPrefix(itemPath, assetPrefix) || '/'
       }
 
       // Simulate minimal mode requests by normalizing RSC and postponed

diff --git a/test/development/basic/asset-prefix/asset-prefix.test.ts b/test/development/basic/asset-prefix/asset-prefix.test.ts
@@ -23,4 +23,18 @@ describe('asset-prefix', () => {
 
     expect(await browser.eval(`window.__v`)).toBe(1)
   })
+
+  describe('rewrites', () => {
+    it('rewrites that do not start with assetPrefix should still work', async () => {
+      const res = await next.fetch('/not-custom-asset-prefix/api/test-json', {})
+      expect(res.status).toBe(200)
+      expect(await res.text()).toBe('{"message":"test"}')
+    })
+
+    it('should respect rewrites that start with assetPrefix', async () => {
+      const res = await next.fetch('/custom-asset-prefix/api/test-json', {})
+      expect(res.status).toBe(200)
+      expect(await res.text()).toBe('{"message":"test"}')
+    })
+  })
 })
diff --git a/test/development/basic/asset-prefix/fixture/next.config.js b/test/development/basic/asset-prefix/fixture/next.config.js
@@ -15,6 +15,16 @@ module.exports = {
           locale: false,
         },
       ],
+      afterFiles: [
+        {
+          source: `/${ASSET_PREFIX}/:path*`,
+          destination: '/:path*',
+        },
+        {
+          source: '/not-custom-asset-prefix/:path*',
+          destination: '/:path*',
+        },
+      ],
     }
   },
 }
diff --git a/test/development/basic/asset-prefix/fixture/pages/api/test-json.js b/test/development/basic/asset-prefix/fixture/pages/api/test-json.js
@@ -0,0 +1,3 @@
+export default function handler(req, res) {
+  res.status(200).json({ message: 'test' })
+}
diff --git a/test/e2e/app-dir/asset-prefix-with-basepath/app/api/test-json/route.js b/test/e2e/app-dir/asset-prefix-with-basepath/app/api/test-json/route.js
@@ -0,0 +1,5 @@
+import { NextResponse } from 'next/server'
+
+export const GET = () => {
+  return NextResponse.json({ message: 'test' })
+}
diff --git a/test/e2e/app-dir/asset-prefix-with-basepath/asset-prefix-with-basepath.test.ts b/test/e2e/app-dir/asset-prefix-with-basepath/asset-prefix-with-basepath.test.ts
@@ -3,7 +3,6 @@ import { nextTestSetup } from 'e2e-utils'
 describe('app-dir assetPrefix with basePath handling', () => {
   const { next } = nextTestSetup({
     files: __dirname,
-    skipDeployment: true,
   })
 
   it('should redirect route when requesting it directly', async () => {
@@ -54,4 +53,24 @@ describe('app-dir assetPrefix with basePath handling', () => {
       expect(status).toBe(200)
     }
   })
+
+  describe('rewrites', () => {
+    it('rewrites that do not start with assetPrefix should still work', async () => {
+      const res = await next.fetch(
+        '/custom-base-path/not-custom-asset-prefix/api/test-json',
+        {}
+      )
+      expect(res.status).toBe(200)
+      expect(await res.text()).toBe('{"message":"test"}')
+    })
+
+    it('should respect rewrites that start with assetPrefix', async () => {
+      const res = await next.fetch(
+        '/custom-base-path/custom-asset-prefix/api/test-json',
+        {}
+      )
+      expect(res.status).toBe(200)
+      expect(await res.text()).toBe('{"message":"test"}')
+    })
+  })
 })
diff --git a/test/e2e/app-dir/asset-prefix-with-basepath/next.config.js b/test/e2e/app-dir/asset-prefix-with-basepath/next.config.js
@@ -1,4 +1,16 @@
 module.exports = {
   assetPrefix: '/custom-asset-prefix',
   basePath: '/custom-base-path',
+  async rewrites() {
+    return [
+      {
+        source: '/custom-asset-prefix/:path*',
+        destination: '/:path*',
+      },
+      {
+        source: '/not-custom-asset-prefix/:path*',
+        destination: '/:path*',
+      },
+    ]
+  },
 }
diff --git a/test/e2e/app-dir/asset-prefix/app/api/test-json/route.js b/test/e2e/app-dir/asset-prefix/app/api/test-json/route.js
@@ -0,0 +1,5 @@
+import { NextResponse } from 'next/server'
+
+export const GET = () => {
+  return NextResponse.json({ message: 'test' })
+}
diff --git a/test/e2e/app-dir/asset-prefix/asset-prefix.test.ts b/test/e2e/app-dir/asset-prefix/asset-prefix.test.ts
@@ -3,7 +3,6 @@ import { nextTestSetup } from 'e2e-utils'
 describe('app-dir assetPrefix handling', () => {
   const { next } = nextTestSetup({
     files: __dirname,
-    skipDeployment: true,
   })
 
   it('should redirect route when requesting it directly', async () => {
@@ -52,4 +51,18 @@ describe('app-dir assetPrefix handling', () => {
       expect(status).toBe(200)
     })
   })
+
+  describe('rewrites', () => {
+    it('rewrites that do not start with assetPrefix should still work', async () => {
+      const res = await next.fetch('/not-custom-asset-prefix/api/test-json', {})
+      expect(res.status).toBe(200)
+      expect(await res.text()).toBe('{"message":"test"}')
+    })
+
+    it('should respect rewrites that start with assetPrefix', async () => {
+      const res = await next.fetch('/custom-asset-prefix/api/test-json', {})
+      expect(res.status).toBe(200)
+      expect(await res.text()).toBe('{"message":"test"}')
+    })
+  })
 })
diff --git a/test/e2e/app-dir/asset-prefix/next.config.js b/test/e2e/app-dir/asset-prefix/next.config.js
@@ -1,3 +1,17 @@
 module.exports = {
   assetPrefix: '/custom-asset-prefix',
+  async rewrites() {
+    return {
+      afterFiles: [
+        {
+          source: '/custom-asset-prefix/:path*',
+          destination: '/:path*',
+        },
+        {
+          source: '/not-custom-asset-prefix/:path*',
+          destination: '/:path*',
+        },
+      ],
+    }
+  },
 }