From d871cd77b8fccaff6121c8de69f9979d62578812 Mon Sep 17 00:00:00 2001 From: Matt Kane Date: Wed, 18 Jan 2023 18:34:22 +0000 Subject: [PATCH 1/2] fix: match edge runtime pages with optional trailing slash --- packages/runtime/src/helpers/edge.ts | 132 +++++++++++------- .../streaming-ssr/index.test.ts | 2 +- 2 files changed, 82 insertions(+), 52 deletions(-) diff --git a/packages/runtime/src/helpers/edge.ts b/packages/runtime/src/helpers/edge.ts index 8c1c2a66d5..9268d5d3d7 100644 --- a/packages/runtime/src/helpers/edge.ts +++ b/packages/runtime/src/helpers/edge.ts @@ -202,27 +202,16 @@ const writeEdgeFunction = async ({ edgeFunctionDefinition, edgeFunctionRoot, netlifyConfig, - pageRegexMap, - appPathRoutesManifest = {}, - nextConfig, - cache, + functionName, + matchers, }: { edgeFunctionDefinition: EdgeFunctionDefinition edgeFunctionRoot: string netlifyConfig: NetlifyConfig - pageRegexMap?: Map - appPathRoutesManifest?: Record - nextConfig: NextConfig - cache?: 'manual' -}): Promise< - Array<{ - function: string - name: string - pattern: string - }> -> => { - const name = sanitizeName(edgeFunctionDefinition.name) - const edgeFunctionDir = join(edgeFunctionRoot, name) + functionName: string + matchers: Array +}) => { + const edgeFunctionDir = join(edgeFunctionRoot, functionName) const bundle = await getMiddlewareBundle({ edgeFunctionDefinition, @@ -238,39 +227,45 @@ const writeEdgeFunction = async ({ target: 'index.ts', }) - const matchers: EdgeFunctionDefinitionV2['matchers'] = [] + await writeJson(join(edgeFunctionDir, 'matchers.json'), matchers) +} +const generateEdgeFunctionMiddlewareMatchers = ({ + edgeFunctionDefinition, + nextConfig, +}: { + edgeFunctionDefinition: EdgeFunctionDefinition + edgeFunctionRoot: string + nextConfig: NextConfig + cache?: 'manual' +}): Array => { // The v1 middleware manifest has a single regexp, but the v2 has an array of matchers if ('regexp' in edgeFunctionDefinition) { - matchers.push({ regexp: edgeFunctionDefinition.regexp }) - } else if (nextConfig.i18n) { - matchers.push( - ...edgeFunctionDefinition.matchers.map((matcher) => ({ - ...matcher, - regexp: makeLocaleOptional(matcher.regexp), - })), - ) - } else { - matchers.push(...edgeFunctionDefinition.matchers) + return [{ regexp: edgeFunctionDefinition.regexp }] } - - // If the EF matches a page, it's an app dir page so needs a matcher too - // The object will be empty if appDir isn't enabled in the Next config - if (pageRegexMap && edgeFunctionDefinition.page in appPathRoutesManifest) { - const regexp = pageRegexMap.get(appPathRoutesManifest[edgeFunctionDefinition.page]) - if (regexp) { - matchers.push({ regexp }) - } + if (nextConfig.i18n) { + return edgeFunctionDefinition.matchers.map((matcher) => ({ + ...matcher, + regexp: makeLocaleOptional(matcher.regexp), + })) } + return edgeFunctionDefinition.matchers +} - await writeJson(join(edgeFunctionDir, 'matchers.json'), matchers) - - // We add a defintion for each matching path - return matchers.map((matcher) => { - const pattern = transformCaptureGroups(stripLookahead(matcher.regexp)) - return { function: name, pattern, name: edgeFunctionDefinition.name, cache } - }) +const middlewareMatcherToEdgeFunctionDefinition = ( + matcher: MiddlewareMatcher, + name: string, + cache?: 'manual', +): { + function: string + name?: string + pattern: string + cache?: 'manual' +} => { + const pattern = transformCaptureGroups(stripLookahead(matcher.regexp)) + return { function: name, pattern, name, cache } } + export const cleanupEdgeFunctions = ({ INTERNAL_EDGE_FUNCTIONS_SRC = '.netlify/edge-functions', }: NetlifyPluginConstants) => emptyDir(INTERNAL_EDGE_FUNCTIONS_SRC) @@ -348,9 +343,27 @@ export const writeRscDataEdgeFunction = async ({ ] } +const getEdgeFunctionPatternForPage = ({ + edgeFunctionDefinition, + pageRegexMap, + appPathRoutesManifest, +}: { + edgeFunctionDefinition: EdgeFunctionDefinitionV2 + pageRegexMap: Map + appPathRoutesManifest?: Record +}): string => { + // We don't just use the matcher from the edge function definition, because it doesn't handle trailing slashes + + // appDir functions have a name that _isn't_ the route name, but rather the route with `/page` appended + const regexp = pageRegexMap.get(appPathRoutesManifest?.[edgeFunctionDefinition.page] ?? edgeFunctionDefinition.page) + return regexp ?? edgeFunctionDefinition.matchers[0].regexp +} + /** * Writes Edge Functions for the Next middleware */ + +// eslint-disable-next-line max-lines-per-function export const writeEdgeFunctions = async ({ netlifyConfig, routesManifest, @@ -415,16 +428,24 @@ export const writeEdgeFunctions = async ({ for (const middleware of middlewareManifest.sortedMiddleware) { usesEdge = true const edgeFunctionDefinition = middlewareManifest.middleware[middleware] - const functionDefinitions = await writeEdgeFunction({ + const functionName = sanitizeName(edgeFunctionDefinition.name) + const matchers = generateEdgeFunctionMiddlewareMatchers({ edgeFunctionDefinition, edgeFunctionRoot, - netlifyConfig, nextConfig, }) - manifest.functions.push(...functionDefinitions) + await writeEdgeFunction({ + edgeFunctionDefinition, + edgeFunctionRoot, + netlifyConfig, + functionName, + matchers, + }) + + manifest.functions.push( + ...matchers.map((matcher) => middlewareMatcherToEdgeFunctionDefinition(matcher, functionName)), + ) } - // Older versions of the manifest format don't have the functions field - // No, the version field was not incremented if (typeof middlewareManifest.functions === 'object') { // When using the app dir, we also need to check if the EF matches a page const appPathRoutesManifest = await loadAppPathRoutesManifest(netlifyConfig) @@ -438,17 +459,26 @@ export const writeEdgeFunctions = async ({ for (const edgeFunctionDefinition of Object.values(middlewareManifest.functions)) { usesEdge = true - const functionDefinitions = await writeEdgeFunction({ + const functionName = sanitizeName(edgeFunctionDefinition.name) + await writeEdgeFunction({ edgeFunctionDefinition, edgeFunctionRoot, netlifyConfig, + functionName, + matchers: edgeFunctionDefinition.matchers, + }) + const pattern = getEdgeFunctionPatternForPage({ + edgeFunctionDefinition, pageRegexMap, appPathRoutesManifest, - nextConfig, + }) + manifest.functions.push({ + function: functionName, + name: edgeFunctionDefinition.name, + pattern, // cache: "manual" is currently experimental, so we restrict it to sites that use experimental appDir cache: usesAppDir ? 'manual' : undefined, }) - manifest.functions.push(...functionDefinitions) } } if (usesEdge) { diff --git a/test/e2e/modified-tests/streaming-ssr/index.test.ts b/test/e2e/modified-tests/streaming-ssr/index.test.ts index d20d8ac711..53b42d23b4 100644 --- a/test/e2e/modified-tests/streaming-ssr/index.test.ts +++ b/test/e2e/modified-tests/streaming-ssr/index.test.ts @@ -43,7 +43,7 @@ describe('react 18 streaming SSR with custom next configs', () => { expect(html).toContain('color:blue') }) // NTL Skip - usuallySkip('should redirect paths without trailing-slash and render when slash is appended', async () => { + it('should redirect paths without trailing-slash and render when slash is appended', async () => { const page = '/hello' const redirectRes = await fetchViaHTTP(next.url, page, {}, { redirect: 'manual' }) const res = await fetchViaHTTP(next.url, page + '/') From 747af5de4dae7fbe02094b2eb66e7fecff0c0ca1 Mon Sep 17 00:00:00 2001 From: Matt Kane Date: Thu, 19 Jan 2023 11:54:20 +0000 Subject: [PATCH 2/2] fix: handle trailing slash redirects --- packages/runtime/src/helpers/edge.ts | 15 ++-- .../src/templates/edge-shared/utils.test.ts | 52 ++++++++++++- .../src/templates/edge-shared/utils.ts | 75 +++++++++++++++++++ .../src/templates/edge/function-runtime.ts | 23 ++++++ .../{runtime.ts => middleware-runtime.ts} | 59 +-------------- 5 files changed, 162 insertions(+), 62 deletions(-) create mode 100644 packages/runtime/src/templates/edge/function-runtime.ts rename packages/runtime/src/templates/edge/{runtime.ts => middleware-runtime.ts} (58%) diff --git a/packages/runtime/src/helpers/edge.ts b/packages/runtime/src/helpers/edge.ts index 9268d5d3d7..17041426c9 100644 --- a/packages/runtime/src/helpers/edge.ts +++ b/packages/runtime/src/helpers/edge.ts @@ -203,13 +203,15 @@ const writeEdgeFunction = async ({ edgeFunctionRoot, netlifyConfig, functionName, - matchers, + matchers = [], + middleware = false, }: { edgeFunctionDefinition: EdgeFunctionDefinition edgeFunctionRoot: string netlifyConfig: NetlifyConfig functionName: string - matchers: Array + matchers?: Array + middleware?: boolean }) => { const edgeFunctionDir = join(edgeFunctionRoot, functionName) @@ -223,11 +225,14 @@ const writeEdgeFunction = async ({ await copyEdgeSourceFile({ edgeFunctionDir, - file: 'runtime.ts', + file: middleware ? 'middleware-runtime.ts' : 'function-runtime.ts', target: 'index.ts', }) - await writeJson(join(edgeFunctionDir, 'matchers.json'), matchers) + if (middleware) { + // Functions don't have complex matchers, so we can rely on the Netlify matcher + await writeJson(join(edgeFunctionDir, 'matchers.json'), matchers) + } } const generateEdgeFunctionMiddlewareMatchers = ({ @@ -440,6 +445,7 @@ export const writeEdgeFunctions = async ({ netlifyConfig, functionName, matchers, + middleware: true, }) manifest.functions.push( @@ -465,7 +471,6 @@ export const writeEdgeFunctions = async ({ edgeFunctionRoot, netlifyConfig, functionName, - matchers: edgeFunctionDefinition.matchers, }) const pattern = getEdgeFunctionPatternForPage({ edgeFunctionDefinition, diff --git a/packages/runtime/src/templates/edge-shared/utils.test.ts b/packages/runtime/src/templates/edge-shared/utils.test.ts index 9c988f6816..b7f835b33e 100644 --- a/packages/runtime/src/templates/edge-shared/utils.test.ts +++ b/packages/runtime/src/templates/edge-shared/utils.test.ts @@ -1,6 +1,6 @@ import { assertEquals } from 'https://deno.land/std@0.167.0/testing/asserts.ts' import { beforeEach, describe, it } from 'https://deno.land/std@0.167.0/testing/bdd.ts' -import { updateModifiedHeaders, FetchEventResult } from './utils.ts' +import { redirectTrailingSlash, updateModifiedHeaders } from './utils.ts' describe('updateModifiedHeaders', () => { it("does not modify the headers if 'x-middleware-override-headers' is not found", () => { @@ -62,3 +62,53 @@ describe('updateModifiedHeaders', () => { }) }) }) + +describe('trailing slash redirects', () => { + it('adds a trailing slash to the pathn if trailingSlash is enabled', () => { + const url = new URL('https://example.com/foo') + const result = redirectTrailingSlash(url, true) + assertEquals(result?.status, 308) + assertEquals(result?.headers.get('location'), 'https://example.com/foo/') + }) + + it("doesn't add a trailing slash if trailingSlash is false", () => { + const url = new URL('https://example.com/foo') + const result = redirectTrailingSlash(url, false) + assertEquals(result, undefined) + }) + + it("doesn't add a trailing slash if the path is a file", () => { + const url = new URL('https://example.com/foo.txt') + const result = redirectTrailingSlash(url, true) + assertEquals(result, undefined) + }) + it('adds a trailing slash if there is a dot in the path', () => { + const url = new URL('https://example.com/foo.bar/baz') + const result = redirectTrailingSlash(url, true) + assertEquals(result?.status, 308) + assertEquals(result?.headers.get('location'), 'https://example.com/foo.bar/baz/') + }) + it("doesn't add a trailing slash if the path is /", () => { + const url = new URL('https://example.com/') + const result = redirectTrailingSlash(url, true) + assertEquals(result, undefined) + }) + it('removes a trailing slash from the path if trailingSlash is false', () => { + const url = new URL('https://example.com/foo/') + const result = redirectTrailingSlash(url, false) + assertEquals(result?.status, 308) + assertEquals(result?.headers.get('location'), 'https://example.com/foo') + }) + it("doesn't remove a trailing slash if trailingSlash is true", () => { + const url = new URL('https://example.com/foo/') + const result = redirectTrailingSlash(url, true) + assertEquals(result, undefined) + }) + + it('removes a trailing slash from the path if the path is a file', () => { + const url = new URL('https://example.com/foo.txt/') + const result = redirectTrailingSlash(url, false) + assertEquals(result?.status, 308) + assertEquals(result?.headers.get('location'), 'https://example.com/foo.txt') + }) +}) diff --git a/packages/runtime/src/templates/edge-shared/utils.ts b/packages/runtime/src/templates/edge-shared/utils.ts index 98980c1b7b..fb09b18aba 100644 --- a/packages/runtime/src/templates/edge-shared/utils.ts +++ b/packages/runtime/src/templates/edge-shared/utils.ts @@ -56,6 +56,37 @@ interface MiddlewareRequest { rewrite(destination: string | URL, init?: ResponseInit): Response } +export interface I18NConfig { + defaultLocale: string + localeDetection?: false + locales: string[] +} + +export interface RequestData { + geo?: { + city?: string + country?: string + region?: string + latitude?: string + longitude?: string + timezone?: string + } + headers: Record + ip?: string + method: string + nextConfig?: { + basePath?: string + i18n?: I18NConfig | null + trailingSlash?: boolean + } + page?: { + name?: string + params?: { [key: string]: string } + } + url: string + body?: ReadableStream +} + function isMiddlewareRequest(response: Response | MiddlewareRequest): response is MiddlewareRequest { return 'originalRequest' in response } @@ -90,6 +121,34 @@ export function updateModifiedHeaders(requestHeaders: Headers, responseHeaders: responseHeaders.delete('x-middleware-override-headers') } +export const buildNextRequest = ( + request: Request, + context: Context, + nextConfig?: RequestData['nextConfig'], +): RequestData => { + const { url, method, body, headers } = request + + const { country, subdivision, city, latitude, longitude, timezone } = context.geo + + const geo: RequestData['geo'] = { + country: country?.code, + region: subdivision?.code, + city, + latitude: latitude?.toString(), + longitude: longitude?.toString(), + timezone, + } + + return { + headers: Object.fromEntries(headers.entries()), + geo, + url, + method, + ip: context.ip, + body: body ?? undefined, + nextConfig, + } +} export const buildResponse = async ({ result, request, @@ -196,3 +255,19 @@ export const buildResponse = async ({ } return res } + +export const redirectTrailingSlash = (url: URL, trailingSlash: boolean): Response | undefined => { + const { pathname } = url + if (pathname === '/') { + return + } + if (!trailingSlash && pathname.endsWith('/')) { + url.pathname = pathname.slice(0, -1) + return Response.redirect(url, 308) + } + // Add a slash, unless there's a file extension + if (trailingSlash && !pathname.endsWith('/') && !pathname.split('/').pop()?.includes('.')) { + url.pathname = `${pathname}/` + return Response.redirect(url, 308) + } +} diff --git a/packages/runtime/src/templates/edge/function-runtime.ts b/packages/runtime/src/templates/edge/function-runtime.ts new file mode 100644 index 0000000000..9d8513d904 --- /dev/null +++ b/packages/runtime/src/templates/edge/function-runtime.ts @@ -0,0 +1,23 @@ +import type { Context } from 'https://edge.netlify.com' +// Available at build time +import edgeFunction from './bundle.js' +import { buildNextRequest, buildResponse, redirectTrailingSlash } from '../edge-shared/utils.ts' +import nextConfig from '../edge-shared/nextConfig.json' assert { type: 'json' } + +const handler = async (req: Request, context: Context) => { + const url = new URL(req.url) + const redirect = redirectTrailingSlash(url, nextConfig.trailingSlash) + if (redirect) { + return redirect + } + const request = buildNextRequest(req, context, nextConfig) + try { + const result = await edgeFunction({ request }) + return buildResponse({ result, request: req, context }) + } catch (error) { + console.error(error) + return new Response(error.message, { status: 500 }) + } +} + +export default handler diff --git a/packages/runtime/src/templates/edge/runtime.ts b/packages/runtime/src/templates/edge/middleware-runtime.ts similarity index 58% rename from packages/runtime/src/templates/edge/runtime.ts rename to packages/runtime/src/templates/edge/middleware-runtime.ts index 45c25eb94a..e61390034e 100644 --- a/packages/runtime/src/templates/edge/runtime.ts +++ b/packages/runtime/src/templates/edge/middleware-runtime.ts @@ -1,49 +1,13 @@ import type { Context } from 'https://edge.netlify.com' // Available at build time import matchers from './matchers.json' assert { type: 'json' } -import nextConfig from '../edge-shared/nextConfig.json' assert { type: 'json' } import edgeFunction from './bundle.js' -import { buildResponse } from '../edge-shared/utils.ts' +import { buildNextRequest, buildResponse } from '../edge-shared/utils.ts' import { getMiddlewareRouteMatcher, MiddlewareRouteMatch, searchParamsToUrlQuery } from '../edge-shared/next-utils.ts' +import nextConfig from '../edge-shared/nextConfig.json' assert { type: 'json' } const matchesMiddleware: MiddlewareRouteMatch = getMiddlewareRouteMatcher(matchers || []) -export interface FetchEventResult { - response: Response - waitUntil: Promise -} - -export interface I18NConfig { - defaultLocale: string - localeDetection?: false - locales: string[] -} - -export interface RequestData { - geo?: { - city?: string - country?: string - region?: string - latitude?: string - longitude?: string - timezone?: string - } - headers: Record - ip?: string - method: string - nextConfig?: { - basePath?: string - i18n?: I18NConfig | null - trailingSlash?: boolean - } - page?: { - name?: string - params?: { [key: string]: string } - } - url: string - body?: ReadableStream -} - export interface RequestContext { request: Request context: Context @@ -70,15 +34,6 @@ const handler = async (req: Request, context: Context) => { return } - const geo: RequestData['geo'] = { - country: context.geo.country?.code, - region: context.geo.subdivision?.code, - city: context.geo.city, - latitude: context.geo.latitude?.toString(), - longitude: context.geo.longitude?.toString(), - timezone: context.geo.timezone, - } - const requestId = req.headers.get('x-nf-request-id') if (!requestId) { console.error('Missing x-nf-request-id header') @@ -89,15 +44,7 @@ const handler = async (req: Request, context: Context) => { }) } - const request: RequestData = { - headers: Object.fromEntries(req.headers.entries()), - geo, - url: req.url, - method: req.method, - ip: context.ip, - body: req.body ?? undefined, - nextConfig, - } + const request = buildNextRequest(req, context, nextConfig) try { const result = await edgeFunction({ request })