fix: middleware i18n normalization (#2483)

* fix: middleware i18n normalization

* test: add new middleware i18n fixture for skipping normalization

* test: more test coverage for middleware i18n and for skipping normalization

* chore: remove test.only

* chore: refactor detectedLocale assignment

Author: orinokai

edge-runtime/lib/next-request.ts

@@ -1,6 +1,12 @@
 import type { Context } from '@netlify/edge-functions'
 
-import { addBasePath, normalizeDataUrl, normalizeLocalePath, removeBasePath } from './util.ts'
+import {
+  addBasePath,
+  addTrailingSlash,
+  normalizeDataUrl,
+  normalizeLocalePath,
+  removeBasePath,
+} from './util.ts'
 
 interface I18NConfig {
   defaultLocale: string
@@ -41,43 +47,25 @@ const normalizeRequestURL = (
 ): { url: string; detectedLocale?: string } => {
   const url = new URL(originalURL)
 
-  url.pathname = removeBasePath(url.pathname, nextConfig?.basePath)
-  const didRemoveBasePath = url.toString() !== originalURL
+  let pathname = removeBasePath(url.pathname, nextConfig?.basePath)
 
-  let detectedLocale: string | undefined
-
-  if (nextConfig?.i18n) {
-    const { pathname, detectedLocale: detected } = normalizeLocalePath(
-      url.pathname,
-      nextConfig?.i18n?.locales,
-    )
-    if (!nextConfig?.skipMiddlewareUrlNormalize) {
-      url.pathname = pathname || '/'
-    }
-    detectedLocale = detected
-  }
+  // If it exists, remove the locale from the URL and store it
+  const { detectedLocale } = normalizeLocalePath(pathname, nextConfig?.i18n?.locales)
 
   if (!nextConfig?.skipMiddlewareUrlNormalize) {
     // We want to run middleware for data requests and expose the URL of the
     // corresponding pages, so we have to normalize the URLs before running
     // the handler.
-    url.pathname = normalizeDataUrl(url.pathname)
+    pathname = normalizeDataUrl(pathname)
 
     // Normalizing the trailing slash based on the `trailingSlash` configuration
     // property from the Next.js config.
-    if (nextConfig?.trailingSlash && url.pathname !== '/' && !url.pathname.endsWith('/')) {
-      url.pathname = `${url.pathname}/`
+    if (nextConfig?.trailingSlash) {
+      pathname = addTrailingSlash(pathname)
     }
   }
 
-  if (didRemoveBasePath) {
-    url.pathname = addBasePath(url.pathname, nextConfig?.basePath)
-  }
-
-  // keep the locale in the url for request.nextUrl object
-  if (detectedLocale) {
-    url.pathname = `/${detectedLocale}${url.pathname}`
-  }
+  url.pathname = addBasePath(pathname, nextConfig?.basePath)
 
   return {
     url: url.toString(),

tests/fixtures/middleware-i18n-skip-normalize/middleware.js

@@ -0,0 +1,91 @@
+import { NextResponse } from 'next/server'
+
+export async function middleware(request) {
+  const url = request.nextUrl
+
+  // this is needed for tests to get the BUILD_ID
+  if (url.pathname.startsWith('/_next/static/__BUILD_ID')) {
+    return NextResponse.next()
+  }
+
+  if (url.pathname === '/old-home') {
+    if (url.searchParams.get('override') === 'external') {
+      return Response.redirect('https://example.vercel.sh')
+    } else {
+      url.pathname = '/new-home'
+      return Response.redirect(url)
+    }
+  }
+
+  if (url.searchParams.get('foo') === 'bar') {
+    url.pathname = '/new-home'
+    url.searchParams.delete('foo')
+    return Response.redirect(url)
+  }
+
+  // Chained redirects
+  if (url.pathname === '/redirect-me-alot') {
+    url.pathname = '/redirect-me-alot-2'
+    return Response.redirect(url)
+  }
+
+  if (url.pathname === '/redirect-me-alot-2') {
+    url.pathname = '/redirect-me-alot-3'
+    return Response.redirect(url)
+  }
+
+  if (url.pathname === '/redirect-me-alot-3') {
+    url.pathname = '/redirect-me-alot-4'
+    return Response.redirect(url)
+  }
+
+  if (url.pathname === '/redirect-me-alot-4') {
+    url.pathname = '/redirect-me-alot-5'
+    return Response.redirect(url)
+  }
+
+  if (url.pathname === '/redirect-me-alot-5') {
+    url.pathname = '/redirect-me-alot-6'
+    return Response.redirect(url)
+  }
+
+  if (url.pathname === '/redirect-me-alot-6') {
+    url.pathname = '/redirect-me-alot-7'
+    return Response.redirect(url)
+  }
+
+  if (url.pathname === '/redirect-me-alot-7') {
+    url.pathname = '/new-home'
+    return Response.redirect(url)
+  }
+
+  // Infinite loop
+  if (url.pathname === '/infinite-loop') {
+    url.pathname = '/infinite-loop-1'
+    return Response.redirect(url)
+  }
+
+  if (url.pathname === '/infinite-loop-1') {
+    url.pathname = '/infinite-loop'
+    return Response.redirect(url)
+  }
+
+  if (url.pathname === '/to') {
+    url.pathname = url.searchParams.get('pathname')
+    url.searchParams.delete('pathname')
+    return Response.redirect(url)
+  }
+
+  if (url.pathname === '/with-fragment') {
+    console.log(String(new URL('/new-home#fragment', url)))
+    return Response.redirect(new URL('/new-home#fragment', url))
+  }
+
+  if (url.pathname.includes('/json')) {
+    return NextResponse.json({
+      requestUrlPathname: new URL(request.url).pathname,
+      nextUrlPathname: request.nextUrl.pathname,
+      nextUrlLocale: request.nextUrl.locale,
+    })
+  }
+}

tests/fixtures/middleware-i18n-skip-normalize/next.config.js

@@ -0,0 +1,24 @@
+module.exports = {
+  output: 'standalone',
+  eslint: {
+    ignoreDuringBuilds: true,
+  },
+  i18n: {
+    locales: ['en', 'fr', 'nl', 'es'],
+    defaultLocale: 'en',
+  },
+  skipMiddlewareUrlNormalize: true,
+  experimental: {
+    clientRouterFilter: true,
+    clientRouterFilterRedirects: true,
+  },
+  redirects() {
+    return [
+      {
+        source: '/to-new',
+        destination: '/dynamic/new',
+        permanent: false,
+      },
+    ]
+  },
+}

tests/fixtures/middleware-i18n-skip-normalize/package.json

@@ -0,0 +1,18 @@
+{
+  "name": "middleware-pages",
+  "version": "0.1.0",
+  "private": true,
+  "scripts": {
+    "postinstall": "next build",
+    "dev": "next dev",
+    "build": "next build"
+  },
+  "dependencies": {
+    "next": "latest",
+    "react": "18.2.0",
+    "react-dom": "18.2.0"
+  },
+  "devDependencies": {
+    "@types/react": "18.2.47"
+  }
+}

tests/fixtures/middleware-i18n-skip-normalize/pages/_app.js

@@ -0,0 +1,6 @@
+export default function App({ Component, pageProps }) {
+  if (!pageProps || typeof pageProps !== 'object') {
+    throw new Error(`Invariant: received invalid pageProps in _app, received ${pageProps}`)
+  }
+  return <Component {...pageProps} />
+}

tests/fixtures/middleware-i18n-skip-normalize/pages/api/ok.js

@@ -0,0 +1,3 @@
+export default function handler(req, res) {
+  res.send('ok')
+}

tests/fixtures/middleware-i18n-skip-normalize/pages/dynamic/[slug].js

@@ -0,0 +1,15 @@
+export default function Account({ slug }) {
+  return (
+    <p id="dynamic" className="title">
+      Welcome to a /dynamic/[slug]: {slug}
+    </p>
+  )
+}
+
+export function getServerSideProps({ params }) {
+  return {
+    props: {
+      slug: params.slug,
+    },
+  }
+}

tests/fixtures/middleware-i18n-skip-normalize/pages/index.js

@@ -0,0 +1,35 @@
+import Link from 'next/link'
+
+export default function Home() {
+  return (
+    <div>
+      <p className="title">Home Page</p>
+      <Link href="/old-home" id="old-home">
+        Redirect me to a new version of a page
+      </Link>
+      <div />
+      <Link href="/old-home?override=external" id="old-home-external">
+        Redirect me to an external site
+      </Link>
+      <div />
+      <Link href="/blank-page?foo=bar">Redirect me with URL params intact</Link>
+      <div />
+      <Link href="/redirect-to-google">Redirect me to Google (with no body response)</Link>
+      <div />
+      <Link href="/redirect-to-google">Redirect me to Google (with no stream response)</Link>
+      <div />
+      <Link href="/redirect-me-alot">Redirect me alot (chained requests)</Link>
+      <div />
+      <Link href="/infinite-loop">Redirect me alot (infinite loop)</Link>
+      <div />
+      <Link href="/to?pathname=/api/ok" locale="nl" id="link-to-api-with-locale">
+        Redirect me to api with locale
+      </Link>
+      <div />
+      <Link href="/to?pathname=/old-home" id="link-to-to-old-home">
+        Redirect me to a redirecting page of new version of page
+      </Link>
+      <div />
+    </div>
+  )
+}

tests/fixtures/middleware-i18n-skip-normalize/pages/new-home.js

@@ -0,0 +1,7 @@
+export default function Account() {
+  return (
+    <p id="new-home-title" className="title">
+      Welcome to a new page
+    </p>
+  )
+}

tests/fixtures/middleware-i18n/middleware.js

@@ -82,6 +82,10 @@ export async function middleware(request) {
   }
 
   if (url.pathname.includes('/json')) {
-    return NextResponse.json({ url: request.nextUrl.href, locale: request.nextUrl.locale })
+    return NextResponse.json({
+      requestUrlPathname: new URL(request.url).pathname,
+      nextUrlPathname: request.nextUrl.pathname,
+      nextUrlLocale: request.nextUrl.locale,
+    })
   }
 }

tests/integration/edge-handler.test.ts

@@ -513,16 +513,92 @@ describe('page router', () => {
       res.end()
     })
     ctx.cleanup?.push(() => origin.stop())
+
     const response = await invokeEdgeFunction(ctx, {
       functions: ['___netlify-edge-handler-middleware'],
       origin,
-      url: `/fr/json`,
+      url: `/json`,
     })
     expect(response.status).toBe(200)
+    const body = await response.json()
+
+    expect(body.requestUrlPathname).toBe('/json')
+    expect(body.nextUrlPathname).toBe('/json')
+    expect(body.nextUrlLocale).toBe('en')
+
+    const responseEn = await invokeEdgeFunction(ctx, {
+      functions: ['___netlify-edge-handler-middleware'],
+      origin,
+      url: `/en/json`,
+    })
+    expect(responseEn.status).toBe(200)
+    const bodyEn = await responseEn.json()
+
+    expect(bodyEn.requestUrlPathname).toBe('/json')
+    expect(bodyEn.nextUrlPathname).toBe('/json')
+    expect(bodyEn.nextUrlLocale).toBe('en')
 
+    const responseFr = await invokeEdgeFunction(ctx, {
+      functions: ['___netlify-edge-handler-middleware'],
+      origin,
+      url: `/fr/json`,
+    })
+    expect(responseFr.status).toBe(200)
+    const bodyFr = await responseFr.json()
+
+    expect(bodyFr.requestUrlPathname).toBe('/fr/json')
+    expect(bodyFr.nextUrlPathname).toBe('/json')
+    expect(bodyFr.nextUrlLocale).toBe('fr')
+  })
+
+  test<FixtureTestContext>('should preserve locale in request.nextUrl with skipMiddlewareUrlNormalize', async (ctx) => {
+    await createFixture('middleware-i18n-skip-normalize', ctx)
+    await runPlugin(ctx)
+    const origin = await LocalServer.run(async (req, res) => {
+      res.write(
+        JSON.stringify({
+          url: req.url,
+          headers: req.headers,
+        }),
+      )
+      res.end()
+    })
+    ctx.cleanup?.push(() => origin.stop())
+
+    const response = await invokeEdgeFunction(ctx, {
+      functions: ['___netlify-edge-handler-middleware'],
+      origin,
+      url: `/json`,
+    })
+    expect(response.status).toBe(200)
     const body = await response.json()
-    const bodyUrl = new URL(body.url)
-    expect(bodyUrl.pathname).toBe('/fr/json')
-    expect(body.locale).toBe('fr')
+
+    expect(body.requestUrlPathname).toBe('/json')
+    expect(body.nextUrlPathname).toBe('/json')
+    expect(body.nextUrlLocale).toBe('en')
+
+    const responseEn = await invokeEdgeFunction(ctx, {
+      functions: ['___netlify-edge-handler-middleware'],
+      origin,
+      url: `/en/json`,
+    })
+    expect(responseEn.status).toBe(200)
+    const bodyEn = await responseEn.json()
+
+    expect(bodyEn.requestUrlPathname).toBe('/en/json')
+    expect(bodyEn.nextUrlPathname).toBe('/json')
+    expect(bodyEn.nextUrlLocale).toBe('en')
+
+    const responseFr = await invokeEdgeFunction(ctx, {
+      functions: ['___netlify-edge-handler-middleware'],
+      origin,
+      url: `/fr/json`,
+    })
+    expect(responseFr.status).toBe(200)
+    const bodyFr = await responseFr.json()
+
+    expect(bodyFr.requestUrlPathname).toBe('/fr/json')
+    expect(bodyFr.nextUrlPathname).toBe('/json')
+    expect(bodyFr.nextUrlLocale).toBe('fr')
   })
 })