From f19b12c42e1da81566294ec571f210c5d70e6ba8 Mon Sep 17 00:00:00 2001 From: David Sheldrick Date: Mon, 26 Feb 2024 12:30:35 +0000 Subject: [PATCH] [dx] Derive vercel routes from react-router config (#2937) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I had some free time at the end of the week so I investigated the idea of deriving the vercel routing config from the react-router config, then storing the derived vercel route info in a jest snapshot, and then loading the jest snapshot during the build script. Seems to work well! ### Change Type - [x] `internal` — Any other changes that don't affect the published package[^2] --- apps/dotcom/package.json | 2 + apps/dotcom/scripts/build.ts | 33 +++- apps/dotcom/spaRouteFilters.ts | 8 - .../src/__snapshots__/routes.test.tsx.snap | 38 ++++ apps/dotcom/src/routes.test.ts | 58 ------ apps/dotcom/src/routes.test.tsx | 185 ++++++++++++++++++ apps/dotcom/tsconfig.json | 3 + yarn.lock | 2 + 8 files changed, 256 insertions(+), 73 deletions(-) delete mode 100644 apps/dotcom/spaRouteFilters.ts create mode 100644 apps/dotcom/src/__snapshots__/routes.test.tsx.snap delete mode 100644 apps/dotcom/src/routes.test.ts create mode 100644 apps/dotcom/src/routes.test.tsx diff --git a/apps/dotcom/package.json b/apps/dotcom/package.json index 1f158738a..4c1e17f96 100644 --- a/apps/dotcom/package.json +++ b/apps/dotcom/package.json @@ -38,6 +38,7 @@ "devDependencies": { "@jest/globals": "30.0.0-alpha.2", "@sentry/cli": "^2.25.0", + "@tldraw/validate": "workspace:*", "@types/qrcode": "^1.5.0", "@types/react": "^18.2.47", "@typescript-eslint/utils": "^5.59.0", @@ -45,6 +46,7 @@ "dotenv": "^16.3.1", "eslint": "^8.37.0", "fast-glob": "^3.3.1", + "json5": "^2.2.3", "lazyrepo": "0.0.0-alpha.27", "vite": "^5.0.0", "ws": "^8.16.0" diff --git a/apps/dotcom/scripts/build.ts b/apps/dotcom/scripts/build.ts index b2a327779..e9e734921 100644 --- a/apps/dotcom/scripts/build.ts +++ b/apps/dotcom/scripts/build.ts @@ -4,8 +4,30 @@ import { exec } from '../../../scripts/lib/exec' import { Config } from './vercel-output-config' import { config } from 'dotenv' +import json5 from 'json5' import { nicelog } from '../../../scripts/lib/nicelog' -import { SPA_ROUTE_FILTERS } from '../spaRouteFilters' + +import { T } from '@tldraw/validate' + +// We load the list of routes that should be forwarded to our SPA's index.html here. +// It uses a jest snapshot file because deriving the set of routes from our +// react-router config works fine in our test environment, but is tricky to get running in this +// build script environment for various reasons (no global React, tsx being weird about decorators, etc). +function loadSpaRoutes() { + // eslint-disable-next-line @typescript-eslint/no-var-requires + const routesJson = require('../src/__snapshots__/routes.test.tsx.snap')['the_routes 1'] + const routes = T.arrayOf( + T.object({ + reactRouterPattern: T.string, + vercelRouterPattern: T.string, + }) + ).validate(json5.parse(routesJson)) + return routes.map((route) => ({ + check: true, + src: route.vercelRouterPattern, + dest: '/index.html', + })) +} config({ path: './.env.local', @@ -14,6 +36,9 @@ config({ nicelog('The multiplayer server is', process.env.MULTIPLAYER_SERVER) async function build() { + // make sure we have the latest routes + await exec('yarn', ['test', 'src/routes.test.tsx']) + const spaRoutes = loadSpaRoutes() await exec('vite', ['build', '--emptyOutDir']) await exec('yarn', ['run', '-T', 'sentry-cli', 'sourcemaps', 'inject', 'dist/assets']) // Clear output static folder (in case we are running locally and have already built the app once before) @@ -22,12 +47,6 @@ async function build() { await exec('cp', ['-r', 'dist', '.vercel/output/static']) await exec('rm', ['-rf', ...glob.sync('.vercel/output/static/**/*.js.map')]) - const spaRoutes = SPA_ROUTE_FILTERS.map((route) => ({ - check: true, - src: route, - dest: '/index.html', - })) - writeFileSync( '.vercel/output/config.json', JSON.stringify( diff --git a/apps/dotcom/spaRouteFilters.ts b/apps/dotcom/spaRouteFilters.ts deleted file mode 100644 index 910dfab6a..000000000 --- a/apps/dotcom/spaRouteFilters.ts +++ /dev/null @@ -1,8 +0,0 @@ -// This value is used to configure Vercel routing to rewrite dotcom SPA routes to /index.html. -// It is also tested in routes.test.ts to make sure it matches all React Router routes. -// -// It is a list of string-encoded regexes matching SPA routes to be spliced into -// Vercel's "build output spec" in scripts/build.ts. -// -// Make sure it's not overly broad, because otherwise we won't give correct 404 responses. -export const SPA_ROUTE_FILTERS = ['^/$', '^/r/?$', '^/new/?$', '^/r/.*', '^/s/.*', '^/v/.*'] diff --git a/apps/dotcom/src/__snapshots__/routes.test.tsx.snap b/apps/dotcom/src/__snapshots__/routes.test.tsx.snap new file mode 100644 index 000000000..db16700e1 --- /dev/null +++ b/apps/dotcom/src/__snapshots__/routes.test.tsx.snap @@ -0,0 +1,38 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`the_routes 1`] = ` +[ + { + "reactRouterPattern": "/", + "vercelRouterPattern": "^//?$", + }, + { + "reactRouterPattern": "/new", + "vercelRouterPattern": "^/new/?$", + }, + { + "reactRouterPattern": "/r", + "vercelRouterPattern": "^/r/?$", + }, + { + "reactRouterPattern": "/r/:boardId/history", + "vercelRouterPattern": "^/r/[^/]*/history/?$", + }, + { + "reactRouterPattern": "/r/:boardId/history/:timestamp", + "vercelRouterPattern": "^/r/[^/]*/history/[^/]*/?$", + }, + { + "reactRouterPattern": "/r/:roomId", + "vercelRouterPattern": "^/r/[^/]*/?$", + }, + { + "reactRouterPattern": "/s/:roomId", + "vercelRouterPattern": "^/s/[^/]*/?$", + }, + { + "reactRouterPattern": "/v/:roomId", + "vercelRouterPattern": "^/v/[^/]*/?$", + }, +] +`; diff --git a/apps/dotcom/src/routes.test.ts b/apps/dotcom/src/routes.test.ts deleted file mode 100644 index e578f5802..000000000 --- a/apps/dotcom/src/routes.test.ts +++ /dev/null @@ -1,58 +0,0 @@ -import { expect } from '@jest/globals' -import type { MatcherFunction } from 'expect' -import { RouteObject } from 'react-router-dom' -import { SPA_ROUTE_FILTERS } from '../spaRouteFilters' -import { router } from './routes' - -const toMatchAny: MatcherFunction<[regexes: unknown]> = function (actual, regexes) { - if ( - typeof actual !== 'string' || - !Array.isArray(regexes) || - regexes.some((regex) => typeof regex !== 'string') - ) { - throw new Error('toMatchAny takes a string and an array of strings') - } - - const pass = regexes.map((regex) => new RegExp(regex)).some((regex) => actual.match(regex)) - if (pass) { - return { - message: () => - `expected ${this.utils.printReceived(actual)} not to match any of the regexes ${this.utils.printExpected(regexes)}`, - pass: true, - } - } else { - return { - message: () => - `expected ${this.utils.printReceived(actual)} to match at least one of the regexes ${this.utils.printExpected(regexes)}`, - pass: false, - } - } -} - -expect.extend({ toMatchAny }) - -function extractContentPaths(routeObject: RouteObject): string[] { - const paths: string[] = [] - - if (routeObject.path && routeObject.path !== '*') { - paths.push(routeObject.path) - } - - if (routeObject.children) { - routeObject.children.forEach((child) => { - paths.push(...extractContentPaths(child)) - }) - } - - return paths -} - -test('SPA_ROUTE_FILTERS match all React routes', () => { - router.flatMap(extractContentPaths).forEach((path) => { - expect(path).toMatchAny(SPA_ROUTE_FILTERS) - }) -}) - -test("SPA_ROUTE_FILTERS don't match assets", () => { - expect('/assets/test.png').not.toMatchAny(SPA_ROUTE_FILTERS) -}) diff --git a/apps/dotcom/src/routes.test.tsx b/apps/dotcom/src/routes.test.tsx new file mode 100644 index 000000000..a68dafb3a --- /dev/null +++ b/apps/dotcom/src/routes.test.tsx @@ -0,0 +1,185 @@ +import { expect } from '@jest/globals' +import type { MatcherFunction } from 'expect' +import { join } from 'path' +import { ReactElement } from 'react' +import { Route, RouteObject, createRoutesFromElements } from 'react-router-dom' +import { router } from './routes' + +const toMatchAny: MatcherFunction<[regexes: unknown]> = function (actual, regexes) { + if ( + typeof actual !== 'string' || + !Array.isArray(regexes) || + regexes.some((regex) => typeof regex !== 'string') + ) { + throw new Error('toMatchAny takes a string and an array of strings') + } + + const pass = regexes.map((regex) => new RegExp(regex)).some((regex) => actual.match(regex)) + if (pass) { + return { + message: () => + `expected ${this.utils.printReceived(actual)} not to match any of the regexes ${this.utils.printExpected(regexes)}`, + pass: true, + } + } else { + return { + message: () => + `expected ${this.utils.printReceived(actual)} to match at least one of the regexes ${this.utils.printExpected(regexes)}`, + pass: false, + } + } +} + +expect.extend({ toMatchAny }) + +function extractContentPaths(routeObject: RouteObject): string[] { + const path = routeObject.path || '' + + const paths: string[] = [] + if ( + path && + (routeObject.element || routeObject.Component || routeObject.lazy || routeObject.loader) + ) { + // This is a contentful route so it gets included on its own. + // Technically not every route with a .lazy or a .loader has content, but we don't have a way to check that + // and it's not a huge deal if they don't 404 correctly. + paths.push(path) + } + if (routeObject.children && routeObject.children.length > 0) { + // this route has children, so we need to recurse + paths.push( + ...routeObject.children.flatMap((child) => + extractContentPaths(child).map((childPath) => join(path, childPath)) + ) + ) + } + + return paths +} + +function convertReactToVercel(path: string): string { + // react-router supports wildcard routes https://reactrouter.com/en/main/route/route#splats + // but we don't use them yet so just fail for now until we need them (if ever) + if (path.endsWith('*')) { + throw new Error(`Wildcard routes like '${path}' are not supported yet (you can add support!)`) + } + // react-router supports optional route segments https://reactrouter.com/en/main/route/route#optional-segments + // but we don't use them yet so just fail for now until we need them (if ever) + if (path.match(/\?\//)) { + throw new Error( + `Optional route segments like in '${path}' are not supported yet (you can add this)` + ) + } + // make sure colons are immediately preceded by a slash + if (path.match(/[^/]:/)) { + throw new Error(`Colons in route segments must be immediately preceded by a slash in '${path}'`) + } + if (!path.startsWith('/')) { + throw new Error(`Route paths must start with a slash, but '${path}' does not`) + } + // Wrap in explicit start and end of string anchors (^ and $) + // and replace :param with [^/]* to match any string of non-slash characters, including the empty string + return '^' + path.replace(/:[^/]+/g, '[^/]*') + '/?$' +} + +const spaRoutes = router + .flatMap(extractContentPaths) + .sort() + // ignore the root catch-all route + .filter((path) => path !== '/*' && path !== '*') + .map((path) => ({ + reactRouterPattern: path, + vercelRouterPattern: convertReactToVercel(path), + })) + +const allvercelRouterPatterns = spaRoutes.map((route) => route.vercelRouterPattern) + +test('the_routes', () => { + expect(spaRoutes).toMatchSnapshot() +}) + +test('all React routes match', () => { + for (const route of spaRoutes) { + expect(route.reactRouterPattern).toMatch(new RegExp(route.vercelRouterPattern)) + for (const otherRoute of spaRoutes) { + if (route === otherRoute) continue + expect(route.reactRouterPattern).not.toMatch(new RegExp(otherRoute.vercelRouterPattern)) + } + } +}) + +test("non-react routes don't match", () => { + // lil smoke test for basic patterns + expect('/').toMatchAny(allvercelRouterPatterns) + expect('/new').toMatchAny(allvercelRouterPatterns) + expect('/r/whatever').toMatchAny(allvercelRouterPatterns) + expect('/r/whatever/').toMatchAny(allvercelRouterPatterns) + + expect('/assets/test.png').not.toMatchAny(allvercelRouterPatterns) + expect('/twitter-social.png').not.toMatchAny(allvercelRouterPatterns) + expect('/robots.txt').not.toMatchAny(allvercelRouterPatterns) + expect('/static/css/index.css').not.toMatchAny(allvercelRouterPatterns) +}) + +test('convertReactToVercel', () => { + expect(() => + convertReactToVercel('/r/:roomId/history?/:timestamp') + ).toThrowErrorMatchingInlineSnapshot( + `"Optional route segments like in '/r/:roomId/history?/:timestamp' are not supported yet (you can add this)"` + ) + expect(() => convertReactToVercel('/r/:roomId/history/*')).toThrowErrorMatchingInlineSnapshot( + `"Wildcard routes like '/r/:roomId/history/*' are not supported yet (you can add support!)"` + ) + expect(() => convertReactToVercel('/r/foo:roomId/history')).toThrowErrorMatchingInlineSnapshot( + `"Colons in route segments must be immediately preceded by a slash in '/r/foo:roomId/history'"` + ) + expect(() => convertReactToVercel('r/:roomId/history')).toThrowErrorMatchingInlineSnapshot( + `"Route paths must start with a slash, but 'r/:roomId/history' does not"` + ) +}) + +function extract(...routes: ReactElement[]) { + return createRoutesFromElements( + {routes.map((r, i) => ({ ...r, key: i.toString() }))} + ) + .flatMap(extractContentPaths) + .sort() +} + +describe('extractContentPaths', () => { + it('only includes routes with content', () => { + expect(extract(} />)).toEqual(['/foo']) + expect(extract( 'hi'} />)).toEqual(['/foo']) + expect( + extract( Promise.resolve({ Component: () => 'foo' })} />) + ).toEqual(['/foo']) + expect(extract( null} />)).toEqual(['/foo']) + expect(extract()).toEqual([]) + expect(extract()).toEqual([]) + expect( + extract( + + + + ) + ).toEqual([]) + }) + it('does not include parent routes without content', () => { + expect( + extract( + + } /> + + ) + ).toEqual(['/foo/bar']) + }) + it('does include parent routes with content', () => { + expect( + extract( + }> + } /> + + ) + ).toEqual(['/foo', '/foo/bar']) + }) +}) diff --git a/apps/dotcom/tsconfig.json b/apps/dotcom/tsconfig.json index 26a728317..2eb327536 100644 --- a/apps/dotcom/tsconfig.json +++ b/apps/dotcom/tsconfig.json @@ -33,6 +33,9 @@ }, { "path": "../../packages/tlsync" + }, + { + "path": "../../packages/validate" } ] } diff --git a/yarn.lock b/yarn.lock index af696fad0..c519eab51 100644 --- a/yarn.lock +++ b/yarn.lock @@ -11851,6 +11851,7 @@ __metadata: "@tldraw/assets": "workspace:*" "@tldraw/tldraw": "workspace:*" "@tldraw/tlsync": "workspace:*" + "@tldraw/validate": "workspace:*" "@types/qrcode": "npm:^1.5.0" "@types/react": "npm:^18.2.47" "@typescript-eslint/utils": "npm:^5.59.0" @@ -11861,6 +11862,7 @@ __metadata: eslint: "npm:^8.37.0" fast-glob: "npm:^3.3.1" idb: "npm:^7.1.1" + json5: "npm:^2.2.3" lazyrepo: "npm:0.0.0-alpha.27" nanoid: "npm:4.0.2" qrcode: "npm:^1.5.1"