From 2d9d649d492b77adeddea0242ab6b7fe73d18079 Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Sun, 12 Jul 2020 14:03:49 -0500 Subject: [PATCH] Add handling for custom-routes with basePath (#15041) This adds handling for custom-routes with `basePath` to automatically add the `basePath` for custom-routes `source` and `destination` unless `basePath: false` is set for the route. Closes: https://github.com/vercel/next.js/issues/14782 --- packages/next/build/index.ts | 11 +++ .../webpack/loaders/next-serverless-loader.ts | 13 ++- packages/next/lib/load-custom-routes.ts | 9 +- .../next/next-server/server/next-server.ts | 41 +++++--- packages/next/next-server/server/render.tsx | 3 +- packages/next/next-server/server/router.ts | 80 +++++++++++++-- packages/next/server/next-dev-server.ts | 21 +++- test/integration/basepath/next.config.js | 62 ++++++++++++ test/integration/basepath/test/index.test.js | 98 ++++++++++++++++++- 9 files changed, 306 insertions(+), 32 deletions(-) diff --git a/packages/next/build/index.ts b/packages/next/build/index.ts index ced0bb7a7f..313a2def1d 100644 --- a/packages/next/build/index.ts +++ b/packages/next/build/index.ts @@ -259,11 +259,22 @@ export default async function build( const buildCustomRoute = ( r: { source: string + basePath?: false statusCode?: number + destination?: string }, type: RouteType ) => { const keys: any[] = [] + + if (r.basePath !== false) { + r.source = `${config.basePath}${r.source}` + + if (r.destination && r.destination.startsWith('/')) { + r.destination = `${config.basePath}${r.destination}` + } + } + const routeRegex = pathToRegexp(r.source, keys, { strict: true, sensitive: false, diff --git a/packages/next/build/webpack/loaders/next-serverless-loader.ts b/packages/next/build/webpack/loaders/next-serverless-loader.ts index 2722771b7c..646c155bf1 100644 --- a/packages/next/build/webpack/loaders/next-serverless-loader.ts +++ b/packages/next/build/webpack/loaders/next-serverless-loader.ts @@ -137,7 +137,9 @@ const nextServerlessLoader: loader.Loader = function () { const { parsedDestination } = prepareDestination( rewrite.destination, params, - parsedUrl.query + parsedUrl.query, + true, + "${basePath}" ) Object.assign(parsedUrl.query, parsedDestination.query, params) @@ -173,6 +175,7 @@ const nextServerlessLoader: loader.Loader = function () { ? ` // always strip the basePath if configured since it is required req.url = req.url.replace(new RegExp('^${basePath}'), '') || '/' + parsedUrl.pathname = parsedUrl.pathname.replace(new RegExp('^${basePath}'), '') || '/' ` : '' @@ -204,13 +207,13 @@ const nextServerlessLoader: loader.Loader = function () { try { await initServer() - ${handleBasePath} - // We need to trust the dynamic route params from the proxy // to ensure we are using the correct values const trustQuery = req.headers['${vercelHeader}'] const parsedUrl = handleRewrites(parse(req.url, true)) + ${handleBasePath} + const params = ${ pageIsDynamicRoute ? ` @@ -296,8 +299,6 @@ const nextServerlessLoader: loader.Loader = function () { export async function renderReqToHTML(req, res, renderMode, _renderOpts, _params) { const fromExport = renderMode === 'export' || renderMode === true; - ${handleBasePath} - const options = { App, Document, @@ -324,6 +325,8 @@ const nextServerlessLoader: loader.Loader = function () { const trustQuery = !getStaticProps && req.headers['${vercelHeader}'] const parsedUrl = handleRewrites(parse(req.url, true)) + ${handleBasePath} + if (parsedUrl.pathname.match(/_next\\/data/)) { _nextData = true parsedUrl.pathname = parsedUrl.pathname diff --git a/packages/next/lib/load-custom-routes.ts b/packages/next/lib/load-custom-routes.ts index 0f08de04a8..126c897e09 100644 --- a/packages/next/lib/load-custom-routes.ts +++ b/packages/next/lib/load-custom-routes.ts @@ -8,6 +8,7 @@ import { export type Rewrite = { source: string destination: string + basePath?: false } export type Redirect = Rewrite & { @@ -17,6 +18,7 @@ export type Redirect = Rewrite & { export type Header = { source: string + basePath?: false headers: Array<{ key: string; value: string }> } @@ -148,10 +150,11 @@ function checkCustomRoutes( allowedKeys = new Set([ 'source', 'destination', + 'basePath', ...(isRedirect ? ['statusCode', 'permanent'] : []), ]) } else { - allowedKeys = new Set(['source', 'headers']) + allowedKeys = new Set(['source', 'headers', 'basePath']) } for (const route of routes) { @@ -171,6 +174,10 @@ function checkCustomRoutes( const invalidKeys = keys.filter((key) => !allowedKeys.has(key)) const invalidParts: string[] = [] + if (typeof route.basePath !== 'undefined' && route.basePath !== false) { + invalidParts.push('`basePath` must be undefined or false') + } + if (!route.source) { invalidParts.push('`source` is missing') } else if (typeof route.source !== 'string') { diff --git a/packages/next/next-server/server/next-server.ts b/packages/next/next-server/server/next-server.ts index 8633120cae..ff0a17ed57 100644 --- a/packages/next/next-server/server/next-server.ts +++ b/packages/next/next-server/server/next-server.ts @@ -253,13 +253,11 @@ export default class Server { const { basePath } = this.nextConfig - // if basePath is set require it be present - if (basePath && !req.url!.startsWith(basePath)) { - return this.render404(req, res, parsedUrl) - } else { - // If replace ends up replacing the full url it'll be `undefined`, meaning we have to default it to `/` - parsedUrl.pathname = parsedUrl.pathname!.replace(basePath, '') || '/' - req.url = req.url!.replace(basePath, '') + if (basePath && req.url?.startsWith(basePath)) { + // store original URL to allow checking if basePath was + // provided or not + ;(req as any)._nextHadBasePath = true + req.url = req.url!.replace(basePath, '') || '/' } res.statusCode = 200 @@ -308,6 +306,7 @@ export default class Server { } protected generateRoutes(): { + basePath: string headers: Route[] rewrites: Route[] fsRoutes: Route[] @@ -445,11 +444,17 @@ export default class Server { ...staticFilesRoute, ] + const getCustomRouteBasePath = (r: { basePath?: false }) => { + return r.basePath !== false && this.renderOpts.dev + ? this.nextConfig.basePath + : '' + } + const getCustomRoute = (r: Rewrite | Redirect | Header, type: RouteType) => ({ ...r, type, - match: getCustomRouteMatcher(r.source), + match: getCustomRouteMatcher(`${getCustomRouteBasePath(r)}${r.source}`), name: type, fn: async (_req, _res, _params, _parsedUrl) => ({ finished: false }), } as Route & Rewrite & Header) @@ -458,7 +463,13 @@ export default class Server { if (!value.includes(':')) { return value } - const { parsedDestination } = prepareDestination(value, params, {}) + const { parsedDestination } = prepareDestination( + value, + params, + {}, + false, + '' + ) if ( !parsedDestination.pathname || @@ -507,7 +518,9 @@ export default class Server { const { parsedDestination } = prepareDestination( redirectRoute.destination, params, - parsedUrl.query + parsedUrl.query, + false, + getCustomRouteBasePath(redirectRoute) ) const updatedDestination = formatUrl(parsedDestination) @@ -531,6 +544,7 @@ export default class Server { const rewrites = this.customRoutes.rewrites.map((rewrite) => { const rewriteRoute = getCustomRoute(rewrite, 'rewrite') return { + ...rewriteRoute, check: true, type: rewriteRoute.type, name: `Rewrite route`, @@ -540,7 +554,8 @@ export default class Server { rewriteRoute.destination, params, parsedUrl.query, - true + true, + getCustomRouteBasePath(rewriteRoute) ) // external rewrite, proxy it @@ -560,8 +575,9 @@ export default class Server { finished: true, } } - ;(req as any)._nextDidRewrite = true ;(req as any)._nextRewroteUrl = newUrl + ;(req as any)._nextDidRewrite = + (req as any)._nextRewroteUrl !== req.url return { finished: false, @@ -618,6 +634,7 @@ export default class Server { catchAllRoute, useFileSystemPublicRoutes, dynamicRoutes: this.dynamicRoutes, + basePath: this.nextConfig.basePath, pageChecker: this.hasPage.bind(this), } } diff --git a/packages/next/next-server/server/render.tsx b/packages/next/next-server/server/render.tsx index fbe6fbf8bc..aad2f02d95 100644 --- a/packages/next/next-server/server/render.tsx +++ b/packages/next/next-server/server/render.tsx @@ -304,8 +304,7 @@ export async function renderToHTML( const headTags = (...args: any) => callMiddleware('headTags', args) - const didRewrite = - (req as any)._nextDidRewrite && (req as any)._nextRewroteUrl !== req.url + const didRewrite = (req as any)._nextDidRewrite const isFallback = !!query.__nextFallback delete query.__nextFallback diff --git a/packages/next/next-server/server/router.ts b/packages/next/next-server/server/router.ts index 0461a33298..f897d5720a 100644 --- a/packages/next/next-server/server/router.ts +++ b/packages/next/next-server/server/router.ts @@ -22,6 +22,7 @@ export type Route = { check?: boolean statusCode?: number name: string + requireBasePath?: false fn: ( req: IncomingMessage, res: ServerResponse, @@ -34,11 +35,14 @@ export type DynamicRoutes = Array<{ page: string; match: RouteMatch }> export type PageChecker = (pathname: string) => Promise +const customRouteTypes = new Set(['rewrite', 'redirect', 'header']) + export const prepareDestination = ( destination: string, params: Params, query: ParsedUrlQuery, - appendParamsToQuery?: boolean + appendParamsToQuery: boolean, + basePath: string ) => { const parsedDestination = parseUrl(destination, true) const destQuery = parsedDestination.query @@ -77,8 +81,12 @@ export const prepareDestination = ( } } + const shouldAddBasePath = destination.startsWith('/') && basePath + try { - newUrl = encodeURI(destinationCompiler(params)) + newUrl = `${shouldAddBasePath ? basePath : ''}${encodeURI( + destinationCompiler(params) + )}` const [pathname, hash] = newUrl.split('#') parsedDestination.pathname = pathname @@ -109,7 +117,12 @@ export const prepareDestination = ( } } +function replaceBasePath(basePath: string, pathname: string) { + return pathname!.replace(basePath, '') || '/' +} + export default class Router { + basePath: string headers: Route[] fsRoutes: Route[] rewrites: Route[] @@ -120,6 +133,7 @@ export default class Router { useFileSystemPublicRoutes: boolean constructor({ + basePath = '', headers = [], fsRoutes = [], rewrites = [], @@ -129,6 +143,7 @@ export default class Router { pageChecker, useFileSystemPublicRoutes, }: { + basePath: string headers: Route[] fsRoutes: Route[] rewrites: Route[] @@ -138,6 +153,7 @@ export default class Router { pageChecker: PageChecker useFileSystemPublicRoutes: boolean }) { + this.basePath = basePath this.headers = headers this.fsRoutes = fsRoutes this.rewrites = rewrites @@ -192,7 +208,8 @@ export default class Router { ? [ { type: 'route', - name: 'Page checker', + name: 'page checker', + requireBasePath: false, match: route('/:path*'), fn: async (checkerReq, checkerRes, params, parsedCheckerUrl) => { const { pathname } = parsedCheckerUrl @@ -218,12 +235,44 @@ export default class Router { // disabled ...(this.useFileSystemPublicRoutes ? [this.catchAllRoute] : []), ] + const originallyHadBasePath = + !this.basePath || (req as any)._nextHadBasePath for (const testRoute of allRoutes) { - const newParams = testRoute.match(parsedUrlUpdated.pathname) + // 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 + const originalPathname = currentPathname + const requireBasePath = testRoute.requireBasePath !== false + const isCustomRoute = customRouteTypes.has(testRoute.type) + + if (!isCustomRoute) { + // If replace ends up replacing the full url it'll be `undefined`, meaning we have to default it to `/` + currentPathname = replaceBasePath(this.basePath, currentPathname!) + } + + const newParams = testRoute.match(currentPathname) // 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 (!isCustomRoute) { + if (!originallyHadBasePath && !(req as any)._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 + } + + parsedUrlUpdated.pathname = currentPathname + } + const result = await testRoute.fn(req, res, newParams, parsedUrlUpdated) // The response was handled @@ -231,6 +280,12 @@ export default class Router { return true } + // since the fs route didn't match we need to re-add the basePath + // to continue checking rewrites with the basePath present + if (!isCustomRoute) { + parsedUrlUpdated.pathname = originalPathname + } + if (result.pathname) { parsedUrlUpdated.pathname = result.pathname } @@ -244,10 +299,15 @@ export default class Router { // check filesystem if (testRoute.check === true) { + const originalFsPathname = parsedUrlUpdated.pathname + const fsPathname = replaceBasePath(this.basePath, originalFsPathname!) + for (const fsRoute of this.fsRoutes) { - const fsParams = fsRoute.match(parsedUrlUpdated.pathname) + const fsParams = fsRoute.match(fsPathname) if (fsParams) { + parsedUrlUpdated.pathname = fsPathname + const fsResult = await fsRoute.fn( req, res, @@ -258,17 +318,17 @@ export default class Router { if (fsResult.finished) { return true } + + parsedUrlUpdated.pathname = originalFsPathname } } - let matchedPage = await memoizedPageChecker( - parsedUrlUpdated.pathname! - ) + let matchedPage = await memoizedPageChecker(fsPathname) // If we didn't match a page check dynamic routes if (!matchedPage) { for (const dynamicRoute of this.dynamicRoutes) { - if (dynamicRoute.match(parsedUrlUpdated.pathname)) { + if (dynamicRoute.match(fsPathname)) { matchedPage = true } } @@ -276,6 +336,8 @@ export default class Router { // Matched a page or dynamic route so render it using catchAllRoute if (matchedPage) { + parsedUrlUpdated.pathname = fsPathname + const pageParams = this.catchAllRoute.match( parsedUrlUpdated.pathname ) diff --git a/packages/next/server/next-dev-server.ts b/packages/next/server/next-dev-server.ts index cd89e573d2..f3218fd580 100644 --- a/packages/next/server/next-dev-server.ts +++ b/packages/next/server/next-dev-server.ts @@ -336,6 +336,17 @@ export default class DevServer extends Server { parsedUrl: UrlWithParsedQuery ): Promise { await this.devReady + + const { basePath } = this.nextConfig + let removedBasePath = false + + if (basePath && parsedUrl.pathname?.startsWith(basePath)) { + // strip basePath before handling dev bundles + // If replace ends up replacing the full url it'll be `undefined`, meaning we have to default it to `/` + parsedUrl.pathname = parsedUrl.pathname!.replace(basePath, '') || '/' + removedBasePath = true + } + const { pathname } = parsedUrl if (pathname!.startsWith('/_next')) { @@ -351,6 +362,13 @@ export default class DevServer extends Server { return } + // re-add basePath before continuing only if we removed it + // so that custom-routes can accurately determine if they should + // match against the basePath or not + if (removedBasePath) { + parsedUrl.pathname = `${basePath}${parsedUrl.pathname}` + } + return super.run(req, res, parsedUrl) } @@ -393,7 +411,8 @@ export default class DevServer extends Server { fsRoutes.push({ match: route('/:path*'), type: 'route', - name: 'Catchall public directory route', + requireBasePath: false, + name: 'catchall public directory route', fn: async (req, res, params, parsedUrl) => { const { pathname } = parsedUrl if (!pathname) { diff --git a/test/integration/basepath/next.config.js b/test/integration/basepath/next.config.js index ac02f33a29..63db8099c5 100644 --- a/test/integration/basepath/next.config.js +++ b/test/integration/basepath/next.config.js @@ -4,4 +4,66 @@ module.exports = { maxInactiveAge: 1000 * 60 * 60, }, basePath: '/docs', + // replace me + async rewrites() { + return [ + { + source: '/rewrite-1', + destination: '/gssp', + }, + { + source: '/rewrite-no-basepath', + destination: '/gssp', + basePath: false, + }, + { + source: '/rewrite/chain-1', + destination: '/rewrite/chain-2', + }, + { + source: '/rewrite/chain-2', + destination: '/gssp', + }, + ] + }, + + async redirects() { + return [ + { + source: '/redirect-1', + destination: '/somewhere-else', + permanent: false, + }, + { + source: '/redirect-no-basepath', + destination: '/another-destination', + permanent: false, + basePath: false, + }, + ] + }, + + async headers() { + return [ + { + source: '/add-header', + headers: [ + { + key: 'x-hello', + value: 'world', + }, + ], + }, + { + source: '/add-header-no-basepath', + basePath: false, + headers: [ + { + key: 'x-hello', + value: 'world', + }, + ], + }, + ] + }, } diff --git a/test/integration/basepath/test/index.test.js b/test/integration/basepath/test/index.test.js index 53d89b7f8e..b18974e18a 100644 --- a/test/integration/basepath/test/index.test.js +++ b/test/integration/basepath/test/index.test.js @@ -20,6 +20,7 @@ import { initNextServerScript, getRedboxSource, hasRedbox, + fetchViaHTTP, } from 'next-test-utils' import fs, { readFileSync, @@ -116,6 +117,98 @@ const runTests = (context, dev = false) => { }) } + it('should rewrite with basePath by default', async () => { + const html = await renderViaHTTP(context.appPort, '/docs/rewrite-1') + expect(html).toContain('getServerSideProps') + }) + + it('should not rewrite without basePath without disabling', async () => { + const res = await fetchViaHTTP(context.appPort, '/rewrite-1') + expect(res.status).toBe(404) + }) + + it('should not rewrite with basePath when set to false', async () => { + // won't 404 as it matches the dynamic [slug] route + const html = await renderViaHTTP( + context.appPort, + '/docs/rewrite-no-basePath' + ) + expect(html).toContain('slug') + }) + + it('should rewrite without basePath when set to false', async () => { + const html = await renderViaHTTP(context.appPort, '/rewrite-no-basePath') + expect(html).toContain('getServerSideProps') + }) + + it('should redirect with basePath by default', async () => { + const res = await fetchViaHTTP( + context.appPort, + '/docs/redirect-1', + undefined, + { + redirect: 'manual', + } + ) + const { pathname } = url.parse(res.headers.get('location') || '') + expect(pathname).toBe('/docs/somewhere-else') + expect(res.status).toBe(307) + }) + + it('should not redirect without basePath without disabling', async () => { + const res = await fetchViaHTTP(context.appPort, '/redirect-1', undefined, { + redirect: 'manual', + }) + expect(res.status).toBe(404) + }) + + it('should not redirect with basePath when set to false', async () => { + // won't 404 as it matches the dynamic [slug] route + const html = await renderViaHTTP( + context.appPort, + '/docs/rewrite-no-basePath' + ) + expect(html).toContain('slug') + }) + + it('should redirect without basePath when set to false', async () => { + const res = await fetchViaHTTP( + context.appPort, + '/redirect-no-basepath', + undefined, + { + redirect: 'manual', + } + ) + const { pathname } = url.parse(res.headers.get('location') || '') + expect(pathname).toBe('/another-destination') + expect(res.status).toBe(307) + }) + + // + it('should add header with basePath by default', async () => { + const res = await fetchViaHTTP(context.appPort, '/docs/add-header') + expect(res.headers.get('x-hello')).toBe('world') + }) + + it('should not add header without basePath without disabling', async () => { + const res = await fetchViaHTTP(context.appPort, '/add-header') + expect(res.headers.get('x-hello')).toBe(null) + }) + + it('should not add header with basePath when set to false', async () => { + const res = await fetchViaHTTP( + context.appPort, + '/docs/add-header-no-basepath' + ) + expect(res.headers.get('x-hello')).toBe(null) + }) + + it('should add header without basePath when set to false', async () => { + const res = await fetchViaHTTP(context.appPort, '/add-header-no-basepath') + expect(res.headers.get('x-hello')).toBe('world') + }) + it('should not update URL for a 404', async () => { const browser = await webdriver(context.appPort, '/missing') const pathname = await browser.eval(() => window.location.pathname) @@ -759,8 +852,9 @@ describe('basePath serverless', () => { const nextConfig = new File(join(appDir, 'next.config.js')) beforeAll(async () => { - await nextConfig.write( - `module.exports = { target: 'experimental-serverless-trace', basePath: '/docs' } ` + await nextConfig.replace( + '// replace me', + `target: 'experimental-serverless-trace',` ) await nextBuild(appDir) context.appPort = await findPort() -- GitLab