From 562e4283f9f68a3d3eac54bdea7389ee97d7357b Mon Sep 17 00:00:00 2001 From: Forrest Date: Mon, 18 Apr 2022 11:16:39 -0700 Subject: [PATCH] Use finally to clean up seen requests (#36222) * try-finally for router execute request references * lint Co-authored-by: JJ Kasper --- packages/next/server/router.ts | 497 +++++++++++++++++---------------- 1 file changed, 257 insertions(+), 240 deletions(-) diff --git a/packages/next/server/router.ts b/packages/next/server/router.ts index 075d307a14cd1dc..51a591e15ce459c 100644 --- a/packages/next/server/router.ts +++ b/packages/next/server/router.ts @@ -146,287 +146,304 @@ export default class Router { ) } this.seenRequests.add(req) - - // memoize page check calls so we don't duplicate checks for pages - const pageChecks: { [name: string]: Promise } = {} - const memoizedPageChecker = async (p: string): Promise => { - p = normalizeLocalePath(p, this.locales).pathname - - if (pageChecks[p] !== undefined) { - return pageChecks[p] + try { + // memoize page check calls so we don't duplicate checks for pages + const pageChecks: { [name: string]: Promise } = {} + const memoizedPageChecker = async (p: string): Promise => { + p = normalizeLocalePath(p, this.locales).pathname + + if (pageChecks[p] !== undefined) { + return pageChecks[p] + } + const result = this.pageChecker(p) + pageChecks[p] = result + return result } - const result = this.pageChecker(p) - pageChecks[p] = result - return result - } - let parsedUrlUpdated = parsedUrl + let parsedUrlUpdated = parsedUrl - const applyCheckTrue = async (checkParsedUrl: NextUrlWithParsedQuery) => { - const originalFsPathname = checkParsedUrl.pathname - const fsPathname = replaceBasePath(originalFsPathname!, this.basePath) + const applyCheckTrue = async (checkParsedUrl: NextUrlWithParsedQuery) => { + const originalFsPathname = checkParsedUrl.pathname + const fsPathname = replaceBasePath(originalFsPathname!, this.basePath) - for (const fsRoute of this.fsRoutes) { - const fsParams = fsRoute.match(fsPathname) + for (const fsRoute of this.fsRoutes) { + const fsParams = fsRoute.match(fsPathname) - if (fsParams) { - checkParsedUrl.pathname = fsPathname + if (fsParams) { + checkParsedUrl.pathname = fsPathname - const fsResult = await fsRoute.fn(req, res, fsParams, checkParsedUrl) + const fsResult = await fsRoute.fn( + req, + res, + fsParams, + checkParsedUrl + ) - if (fsResult.finished) { - return true - } + if (fsResult.finished) { + return true + } - checkParsedUrl.pathname = originalFsPathname + checkParsedUrl.pathname = originalFsPathname + } } - } - let matchedPage = await memoizedPageChecker(fsPathname) - - // If we didn't match a page check dynamic routes - if (!matchedPage) { - const normalizedFsPathname = normalizeLocalePath( - fsPathname, - this.locales - ).pathname - - for (const dynamicRoute of this.dynamicRoutes) { - if (dynamicRoute.match(normalizedFsPathname)) { - matchedPage = true + let matchedPage = await memoizedPageChecker(fsPathname) + + // If we didn't match a page check dynamic routes + if (!matchedPage) { + const normalizedFsPathname = normalizeLocalePath( + fsPathname, + this.locales + ).pathname + + for (const dynamicRoute of this.dynamicRoutes) { + if (dynamicRoute.match(normalizedFsPathname)) { + matchedPage = true + } } } - } - // Matched a page or dynamic route so render it using catchAllRoute - if (matchedPage) { - const pageParams = this.catchAllRoute.match(checkParsedUrl.pathname) - checkParsedUrl.pathname = fsPathname - checkParsedUrl.query._nextBubbleNoFallback = '1' - - const result = await this.catchAllRoute.fn( - req, - res, - pageParams as Params, - checkParsedUrl - ) - return result.finished + // Matched a page or dynamic route so render it using catchAllRoute + if (matchedPage) { + const pageParams = this.catchAllRoute.match(checkParsedUrl.pathname) + checkParsedUrl.pathname = fsPathname + checkParsedUrl.query._nextBubbleNoFallback = '1' + + const result = await this.catchAllRoute.fn( + req, + res, + pageParams as Params, + checkParsedUrl + ) + return result.finished + } } - } - /* - Desired routes order - - headers - - redirects - - Check filesystem (including pages), if nothing found continue - - User rewrites (checking filesystem and pages each match) - */ - - const allRoutes = [ - ...this.headers, - ...this.redirects, - ...this.rewrites.beforeFiles, - ...(this.useFileSystemPublicRoutes && this.catchAllMiddleware - ? [this.catchAllMiddleware] - : []), - ...this.fsRoutes, - // We only check the catch-all route if public page routes hasn't been - // disabled - ...(this.useFileSystemPublicRoutes - ? [ - { - type: 'route', - name: 'page checker', - requireBasePath: false, - match: route('/:path*'), - fn: async (checkerReq, checkerRes, params, parsedCheckerUrl) => { - let { pathname } = parsedCheckerUrl - pathname = removePathTrailingSlash(pathname || '/') - - if (!pathname) { + /* + Desired routes order + - headers + - redirects + - Check filesystem (including pages), if nothing found continue + - User rewrites (checking filesystem and pages each match) + */ + + const allRoutes = [ + ...this.headers, + ...this.redirects, + ...this.rewrites.beforeFiles, + ...(this.useFileSystemPublicRoutes && this.catchAllMiddleware + ? [this.catchAllMiddleware] + : []), + ...this.fsRoutes, + // We only check the catch-all route if public page routes hasn't been + // disabled + ...(this.useFileSystemPublicRoutes + ? [ + { + type: 'route', + name: 'page checker', + requireBasePath: false, + match: route('/:path*'), + fn: async ( + checkerReq, + checkerRes, + params, + parsedCheckerUrl + ) => { + let { pathname } = parsedCheckerUrl + pathname = removePathTrailingSlash(pathname || '/') + + if (!pathname) { + return { finished: false } + } + + if (await memoizedPageChecker(pathname)) { + return this.catchAllRoute.fn( + checkerReq, + checkerRes, + params, + parsedCheckerUrl + ) + } return { finished: false } - } - - if (await memoizedPageChecker(pathname)) { - return this.catchAllRoute.fn( - checkerReq, - checkerRes, - params, - parsedCheckerUrl - ) - } - return { finished: false } - }, - } as Route, - ] - : []), - ...this.rewrites.afterFiles, - ...(this.rewrites.fallback.length - ? [ - { - type: 'route', - name: 'dynamic route/page check', - requireBasePath: false, - match: route('/:path*'), - fn: async ( - _checkerReq, - _checkerRes, - _params, - parsedCheckerUrl - ) => { - return { - finished: await applyCheckTrue(parsedCheckerUrl), - } - }, - } as Route, - ...this.rewrites.fallback, - ] - : []), - - // We only check the catch-all route if public page routes hasn't been - // disabled - ...(this.useFileSystemPublicRoutes ? [this.catchAllRoute] : []), - ] - const originallyHadBasePath = - !this.basePath || getRequestMeta(req, '_nextHadBasePath') - - for (const testRoute of allRoutes) { - // if basePath is being used, the basePath will still be included - // in the pathname here to allow custom-routes to require containing - // it or not, filesystem routes and pages must always include the basePath - // if it is set - let currentPathname = parsedUrlUpdated.pathname as string - const originalPathname = currentPathname - const requireBasePath = testRoute.requireBasePath !== false - const isCustomRoute = customRouteTypes.has(testRoute.type) - const isPublicFolderCatchall = testRoute.name === 'public folder catchall' - const isMiddlewareCatchall = testRoute.name === 'middleware catchall' - const keepBasePath = - isCustomRoute || isPublicFolderCatchall || isMiddlewareCatchall - const keepLocale = isCustomRoute - - const currentPathnameNoBasePath = replaceBasePath( - currentPathname, - this.basePath - ) + }, + } as Route, + ] + : []), + ...this.rewrites.afterFiles, + ...(this.rewrites.fallback.length + ? [ + { + type: 'route', + name: 'dynamic route/page check', + requireBasePath: false, + match: route('/:path*'), + fn: async ( + _checkerReq, + _checkerRes, + _params, + parsedCheckerUrl + ) => { + return { + finished: await applyCheckTrue(parsedCheckerUrl), + } + }, + } as Route, + ...this.rewrites.fallback, + ] + : []), + + // We only check the catch-all route if public page routes hasn't been + // disabled + ...(this.useFileSystemPublicRoutes ? [this.catchAllRoute] : []), + ] + const originallyHadBasePath = + !this.basePath || getRequestMeta(req, '_nextHadBasePath') + + for (const testRoute of allRoutes) { + // if basePath is being used, the basePath will still be included + // in the pathname here to allow custom-routes to require containing + // it or not, filesystem routes and pages must always include the basePath + // if it is set + let currentPathname = parsedUrlUpdated.pathname as string + const originalPathname = currentPathname + const requireBasePath = testRoute.requireBasePath !== false + const isCustomRoute = customRouteTypes.has(testRoute.type) + const isPublicFolderCatchall = + testRoute.name === 'public folder catchall' + const isMiddlewareCatchall = testRoute.name === 'middleware catchall' + const keepBasePath = + isCustomRoute || isPublicFolderCatchall || isMiddlewareCatchall + const keepLocale = isCustomRoute + + const currentPathnameNoBasePath = replaceBasePath( + currentPathname, + this.basePath + ) - if (!keepBasePath) { - currentPathname = currentPathnameNoBasePath - } + if (!keepBasePath) { + currentPathname = currentPathnameNoBasePath + } - const localePathResult = normalizeLocalePath( - currentPathnameNoBasePath, - this.locales - ) + const localePathResult = normalizeLocalePath( + currentPathnameNoBasePath, + this.locales + ) - const activeBasePath = keepBasePath ? this.basePath : '' - - // don't match API routes when they are locale prefixed - // e.g. /api/hello shouldn't match /en/api/hello as a page - // rewrites/redirects can match though - if ( - !isCustomRoute && - localePathResult.detectedLocale && - localePathResult.pathname.match(/^\/api(?:\/|$)/) - ) { - continue - } + const activeBasePath = keepBasePath ? this.basePath : '' - if (keepLocale) { + // don't match API routes when they are locale prefixed + // e.g. /api/hello shouldn't match /en/api/hello as a page + // rewrites/redirects can match though if ( - !testRoute.internal && - parsedUrl.query.__nextLocale && - !localePathResult.detectedLocale + !isCustomRoute && + localePathResult.detectedLocale && + localePathResult.pathname.match(/^\/api(?:\/|$)/) ) { - currentPathname = `${activeBasePath}/${parsedUrl.query.__nextLocale}${ - currentPathnameNoBasePath === '/' ? '' : currentPathnameNoBasePath - }` + continue } - if ( - getRequestMeta(req, '__nextHadTrailingSlash') && - !currentPathname.endsWith('/') - ) { - currentPathname += '/' + if (keepLocale) { + if ( + !testRoute.internal && + parsedUrl.query.__nextLocale && + !localePathResult.detectedLocale + ) { + currentPathname = `${activeBasePath}/${ + parsedUrl.query.__nextLocale + }${ + currentPathnameNoBasePath === '/' ? '' : currentPathnameNoBasePath + }` + } + + if ( + getRequestMeta(req, '__nextHadTrailingSlash') && + !currentPathname.endsWith('/') + ) { + currentPathname += '/' + } + } else { + currentPathname = `${ + getRequestMeta(req, '_nextHadBasePath') ? activeBasePath : '' + }${ + activeBasePath && currentPathnameNoBasePath === '/' + ? '' + : currentPathnameNoBasePath + }` } - } else { - currentPathname = `${ - getRequestMeta(req, '_nextHadBasePath') ? activeBasePath : '' - }${ - activeBasePath && currentPathnameNoBasePath === '/' - ? '' - : currentPathnameNoBasePath - }` - } - let newParams = testRoute.match(currentPathname) + let newParams = testRoute.match(currentPathname) - if (testRoute.has && newParams) { - const hasParams = matchHas(req, testRoute.has, parsedUrlUpdated.query) + if (testRoute.has && newParams) { + const hasParams = matchHas(req, testRoute.has, parsedUrlUpdated.query) - if (hasParams) { - Object.assign(newParams, hasParams) - } else { - newParams = false + if (hasParams) { + Object.assign(newParams, hasParams) + } else { + newParams = false + } } - } - // Check if the match function matched - if (newParams) { - // since we require basePath be present for non-custom-routes we - // 404 here when we matched an fs route - if (!keepBasePath) { - if ( - !originallyHadBasePath && - !getRequestMeta(req, '_nextDidRewrite') - ) { - if (requireBasePath) { - // consider this a non-match so the 404 renders - this.seenRequests.delete(req) - return false + // Check if the match function matched + if (newParams) { + // since we require basePath be present for non-custom-routes we + // 404 here when we matched an fs route + if (!keepBasePath) { + if ( + !originallyHadBasePath && + !getRequestMeta(req, '_nextDidRewrite') + ) { + if (requireBasePath) { + // consider this a non-match so the 404 renders + return false + } + // page checker occurs before rewrites so we need to continue + // to check those since they don't always require basePath + continue } - // page checker occurs before rewrites so we need to continue - // to check those since they don't always require basePath - continue - } - parsedUrlUpdated.pathname = currentPathname - } + parsedUrlUpdated.pathname = currentPathname + } - const result = await testRoute.fn(req, res, newParams, parsedUrlUpdated) + const result = await testRoute.fn( + req, + res, + newParams, + parsedUrlUpdated + ) - // The response was handled - if (result.finished) { - this.seenRequests.delete(req) - return true - } + // The response was handled + if (result.finished) { + return true + } - // since the fs route didn't finish routing we need to re-add the - // basePath to continue checking with the basePath present - if (!keepBasePath) { - parsedUrlUpdated.pathname = originalPathname - } + // since the fs route didn't finish routing we need to re-add the + // basePath to continue checking with the basePath present + if (!keepBasePath) { + parsedUrlUpdated.pathname = originalPathname + } - if (result.pathname) { - parsedUrlUpdated.pathname = result.pathname - } + if (result.pathname) { + parsedUrlUpdated.pathname = result.pathname + } - if (result.query) { - parsedUrlUpdated.query = { - ...getNextInternalQuery(parsedUrlUpdated.query), - ...result.query, + if (result.query) { + parsedUrlUpdated.query = { + ...getNextInternalQuery(parsedUrlUpdated.query), + ...result.query, + } } - } - // check filesystem - if (testRoute.check === true) { - if (await applyCheckTrue(parsedUrlUpdated)) { - this.seenRequests.delete(req) - return true + // check filesystem + if (testRoute.check === true) { + if (await applyCheckTrue(parsedUrlUpdated)) { + return true + } } } } + return false + } finally { + this.seenRequests.delete(req) } - this.seenRequests.delete(req) - return false } }