diff --git a/.env.example b/.env.example index cfc66f3..5d89543 100644 --- a/.env.example +++ b/.env.example @@ -15,4 +15,10 @@ UMAMI_WEBSITE_ID="your-umami-website-id" UMAMI_DOMAINS="your-umami-domains" # comma separated list of domains the umami script should track - https://umami.is/docs/tracker-configuration#data-domains UMAMI_DOMAIN="your-umami-domain" # umami domain - https://umami.is/docs/bypass-ad-blockers#using-environment-variables-self-hosting UMAMI_SCRIPT_NAME="your-umami-script-name" # umami script name - https://umami.is/docs/bypass-ad-blockers#using-environment-variables-self-hosting -UMAMI_PUBLIC_ANALYTICS_URL="your-umami-public-analytics-url" # https://umami.is/docs/enable-share-url \ No newline at end of file +UMAMI_PUBLIC_ANALYTICS_URL="your-umami-public-analytics-url" # https://umami.is/docs/enable-share-url + +# Set to "false" on non-production deploys (staging, previews) so they are not +# indexed and do not compete with production in search results. It emits +# `X-Robots-Tag: noindex, nofollow` plus a tag. Leave it +# unset (or "true") in production. +ALLOW_INDEXING="false" diff --git a/app/entry.server.tsx b/app/entry.server.tsx index 5a07275..854cdb2 100644 --- a/app/entry.server.tsx +++ b/app/entry.server.tsx @@ -1,7 +1,7 @@ import { PassThrough } from 'node:stream' +import { styleText } from 'node:util' import { createReadableStreamFromReadable } from '@react-router/node' import * as Sentry from '@sentry/node' -import chalk from 'chalk' import { isbot } from 'isbot' import { renderToPipeableStream } from 'react-dom/server' import { @@ -12,6 +12,11 @@ import { } from 'react-router' import { getEnv, init } from './utils/env.server' import { NonceProvider } from './utils/nonce-provider' +import { + getRouteErrorCause, + isExpectedReactRouterRouteError, +} from './utils/sentry-event-filters' +import { makeTimings } from './utils/timing.server' export const streamTimeout = 5000 @@ -37,6 +42,9 @@ export default async function handleRequest(...args: DocRequestArgs) { const nonce = request.headers.get('x-csp-nonce') ?? '' return new Promise(async (resolve, reject) => { let didError = false + // NOTE: this timing will only include things that are rendered in the shell + // and will not include suspended components and deferred loaders + const timings = makeTimings('render', 'renderToPipeableStream') const { pipe, abort } = renderToPipeableStream( @@ -50,6 +58,7 @@ export default async function handleRequest(...args: DocRequestArgs) { [callbackName]: () => { const body = new PassThrough() responseHeaders.set('Content-Type', 'text/html') + responseHeaders.append('Server-Timing', timings.toString()) resolve( new Response(createReadableStreamFromReadable(body), { headers: responseHeaders, @@ -88,11 +97,30 @@ export function handleError( if (request.signal.aborted) { return } - if (error instanceof Error) { - console.error(chalk.red(error.stack)) - void Sentry.captureException(error) - } else { - console.error(error) - void Sentry.captureException(error) + + // React Router hands `handleError` an `ErrorResponse` rather than an + // `Error` for anything a route rejected, so the message and stack live on + // `.error`. Report that instead of the wrapper, otherwise Sentry files a + // stackless issue grouped on a synthesised message. This mirrors React + // Router's own default handler. + const reported = getRouteErrorCause(error) + const logReportedError = () => { + if (reported instanceof Error) { + console.error(styleText('red', String(reported.stack))) + } else { + console.error(reported) + } + } + + // Bots and scanners request every URL in the sitemap with methods the + // routes do not handle, and React Router throws for each of those. They are + // expected answers to invalid traffic, not bugs, so they get logged (they + // still show up in `fly logs`) but never reported to Sentry. + if (isExpectedReactRouterRouteError(error)) { + logReportedError() + return } + + logReportedError() + void Sentry.captureException(reported) } diff --git a/app/root.tsx b/app/root.tsx index 8714535..21a58f2 100644 --- a/app/root.tsx +++ b/app/root.tsx @@ -1,4 +1,5 @@ import { + data, href, Link, Links, @@ -7,6 +8,7 @@ import { Scripts, ScrollRestoration, useLoaderData, + useRouteLoaderData, } from 'react-router' import { ClientOnly } from 'remix-utils/client-only' import { HoneypotProvider } from 'remix-utils/honeypot/react' @@ -29,6 +31,7 @@ import { honeypot } from '#app/utils/honeypot.server' import { getDomainUrl, getUrl } from '#app/utils/misc' import { useNonce } from '#app/utils/nonce-provider' import { getSocialMetas } from '#app/utils/seo' +import { makeTimings, time } from '#app/utils/timing.server' import { type Theme } from '#types/index' import { type Route } from './+types/root' @@ -67,20 +70,64 @@ export const meta: Route.MetaFunction = ({ loaderData }) => { ] } -export async function loader({ request }: Route.LoaderArgs) { - const honeyProps = await honeypot.getInputProps() +/** + * Split out of `loader` so `RootLoaderData` can be derived from it. The loader + * wraps this payload in `data()` to attach a `Server-Timing` header, which + * means `Awaited>` is a `DataWithResponseInit` rather + * than the payload. `useLoaderData`/`useRouteLoaderData` unwrap it, but the + * `meta` functions in child routes read the root match's `loaderData` straight + * out of `matches`, where they need the payload type explicitly. + */ +async function getRootData(request: Request) { + const timings = makeTimings('root loader') + const honeyProps = await time(() => honeypot.getInputProps(), { + timings, + type: 'honeypot', + desc: 'get honeypot input props in root', + }) return { - requestInfo: { - hints: getHints(request), - origin: getDomainUrl(request), - path: new URL(request.url).pathname, + timings, + payload: { + requestInfo: { + hints: getHints(request), + origin: getDomainUrl(request), + path: new URL(request.url).pathname, + }, + ENV: getEnv(), + honeyProps, }, - ENV: getEnv(), - honeyProps, } } +export type RootLoaderData = Awaited>['payload'] + +export async function loader({ request }: Route.LoaderArgs) { + const { payload, timings } = await getRootData(request) + + return data(payload, { + headers: { 'Server-Timing': timings.toString() }, + }) +} + +/** + * React Router only puts a loader's response headers on the document response + * if a matched route exports a `headers` function, so without this the root + * loader's `Server-Timing` never leaves the server. `entry.server.tsx` appends + * the render timings to the same header. + */ +export const headers: Route.HeadersFunction = ({ + loaderHeaders, + parentHeaders, +}) => { + const headers = new Headers(parentHeaders) + const serverTiming = loaderHeaders.get('Server-Timing') + if (serverTiming) { + headers.set('Server-Timing', serverTiming) + } + return headers +} + function Document({ children, nonce, @@ -105,6 +152,13 @@ function Document({ + {env.ALLOW_INDEXING === 'false' ? ( + // Staging deploys opt out of indexing with ALLOW_INDEXING=false + // so they do not compete with production in search results. The + // server sends the matching X-Robots-Tag header; this covers + // crawlers that read the document instead. + + ) : null} @@ -324,9 +378,12 @@ function Footer() { export function ErrorBoundary() { const nonce = useNonce() + // The server sends `X-Robots-Tag` on every response including this one, so + // pass the same env to keep the document's robots meta tag in agreement. + const data = useRouteLoaderData('root') return ( - + ) diff --git a/app/routes/_marketing+/talks.tsx b/app/routes/_marketing+/talks.tsx index 234f687..4938b31 100644 --- a/app/routes/_marketing+/talks.tsx +++ b/app/routes/_marketing+/talks.tsx @@ -7,7 +7,7 @@ import { } from '#app/components/highlight' import { LineGlow } from '#app/components/line-glow' import { TalkCard } from '#app/components/talk-card' -import { type loader as rootLoader } from '#app/root' +import { type RootLoaderData } from '#app/root' import { formatDateWithHints } from '#app/utils/client-hints' import { getUrl } from '#app/utils/misc' import { getSocialMetas } from '#app/utils/seo' @@ -20,8 +20,7 @@ const PAGE_DESCRIPTION = export const meta: Route.MetaFunction = ({ matches }) => { const rootMatch = matches.find((m) => m?.id === 'root') - const rootData = rootMatch?.loaderData as - Awaited> | undefined + const rootData = rootMatch?.loaderData as RootLoaderData | undefined const requestInfo = rootData?.requestInfo return [ { title: PAGE_TITLE }, @@ -53,7 +52,14 @@ export default function Talks() { React.useEffect(() => { const raw = location.hash.slice(1) if (!raw) return - const id = decodeURIComponent(raw) + // A malformed percent-encoding in the hash would throw a URIError here, + // in the browser, and take the effect down with it. + let id: string + try { + id = decodeURIComponent(raw) + } catch { + return + } const run = () => { const el = document.getElementById(id) if (el instanceof HTMLLIElement && el.tabIndex === -1) { diff --git a/app/utils/analytics.tsx b/app/utils/analytics.tsx index 54e593d..342f34d 100644 --- a/app/utils/analytics.tsx +++ b/app/utils/analytics.tsx @@ -4,11 +4,17 @@ import { useState, useEffect, useRef } from 'react' import { useRequestInfo } from './request-info' export function usePosthogPageView() { - const { origin, path } = useRequestInfo() + const requestInfo = useRequestInfo() + // No root loader means no request info (see `useRequestInfo`), which happens + // for the error responses React Router short-circuits. There is no page view + // to report in that case, and the hooks below still have to run. + const origin = requestInfo?.origin + const path = requestInfo?.path const [previousLocation, setPreviousLocation] = useState(path) const isInitialRender = useRef(true) useEffect(() => { + if (path === undefined) return try { if (isInitialRender.current) { isInitialRender.current = false diff --git a/app/utils/client-hints.tsx b/app/utils/client-hints.tsx index 7455015..46ce92d 100644 --- a/app/utils/client-hints.tsx +++ b/app/utils/client-hints.tsx @@ -99,7 +99,10 @@ export function formatDateWithHints(isoDate: string, request: Request) { */ export function useHints() { const requestInfo = useRequestInfo() - return requestInfo.hints + // With no root loader there are no request headers to read the cookies from + // (see `useRequestInfo`), so fall back to each hint's own default rather + // than throwing in the middle of the document render. + return requestInfo?.hints ?? getHints() } /** diff --git a/app/utils/env.server.ts b/app/utils/env.server.ts index 64cc380..1493b3c 100644 --- a/app/utils/env.server.ts +++ b/app/utils/env.server.ts @@ -42,6 +42,12 @@ export function getEnv() { UMAMI_DOMAINS: process.env.UMAMI_DOMAINS, UMAMI_SCRIPT_NAME: process.env.UMAMI_SCRIPT_NAME, UMAMI_PUBLIC_ANALYTICS_URL: process.env.UMAMI_PUBLIC_ANALYTICS_URL, + // Setting this to 'false' tells the server to send an + // `X-Robots-Tag: noindex, nofollow` response header and the document to + // render a matching `` tag. Intended for non-production + // deploys (staging, previews) so they do not compete with production in + // search results. Leave it unset or 'true' in production. + ALLOW_INDEXING: process.env.ALLOW_INDEXING, } } diff --git a/app/utils/request-info.ts b/app/utils/request-info.ts index a6dd659..30729f6 100644 --- a/app/utils/request-info.ts +++ b/app/utils/request-info.ts @@ -1,13 +1,17 @@ -import { invariant } from '@epic-web/invariant' import { useRouteLoaderData } from 'react-router' import { type loader as rootLoader } from '#app/root' /** - * @returns the request info from the root loader + * @returns the request info from the root loader, or `undefined` when the root + * loader never ran. + * + * That second case is not hypothetical. React Router short-circuits a request + * with no matching `loader`/`action`, or with an unsupported method, *before* + * any loader runs — so for a 400/405 there is no root data, yet the document + * (and therefore the error boundary) still has to render. Asserting the data is + * present here turned every such request into a 500 with the real cause + * buried, so callers degrade instead. */ export function useRequestInfo() { - const data = useRouteLoaderData('root') - invariant(data?.requestInfo, 'No requestInfo found in root loader') - - return data.requestInfo + return useRouteLoaderData('root')?.requestInfo } diff --git a/app/utils/schemas.ts b/app/utils/schemas.ts index d534919..4de6cc8 100644 --- a/app/utils/schemas.ts +++ b/app/utils/schemas.ts @@ -66,6 +66,18 @@ export const envSchema = z.object({ UMAMI_DOMAINS: z.string(), UMAMI_SCRIPT_NAME: z.string(), UMAMI_PUBLIC_ANALYTICS_URL: z.string().optional(), + // `server/index.ts` reads the same variable with a deliberately lenient + // grammar: only the exact string 'false' opts out of indexing, everything + // else (including unset and '') means indexing is allowed. An enum would + // reject '', '0' and 'FALSE' in `init()`, and because the app bundle is + // imported per request inside `getBuild()` — whose catch swallows the + // failure — a mistyped value would turn into a permanent silent 500 on + // every route instead of a boot error. Normalise instead of rejecting, so + // the two parsers can never disagree. + ALLOW_INDEXING: z + .string() + .optional() + .transform((value) => (value === 'false' ? 'false' : undefined)), }) // Type exports for TypeScript inference diff --git a/app/utils/sentry-event-filters.ts b/app/utils/sentry-event-filters.ts new file mode 100644 index 0000000..3207f54 --- /dev/null +++ b/app/utils/sentry-event-filters.ts @@ -0,0 +1,114 @@ +/** + * Pure helpers for dropping expected Sentry noise from a public site — the + * bots and scanners that hit `/sitemap.xml` and then request every path in it + * with methods the routes do not handle. + * + * React Router throws a 405-style error for a request with no matching + * `loader` or `action`, and for unsupported request methods. Those are expected + * answers to invalid traffic, not bugs, so they should not page anyone. + * + * NOTE: Healthcheck noise is *not* handled here. `server/utils/monitoring.ts` + * drops those transactions in its `tracesSampler` and the client sets + * `denyUrls`, so a filter for them would be dead code. + */ + +import { isRouteErrorResponse } from 'react-router' + +export const EXPECTED_REACT_ROUTER_ERROR_PATTERNS = [ + /did not provide an `action`/i, + /did not provide a `loader`/i, + /^Invalid request method /i, +] + +/** + * The only statuses React Router uses for these three errors: 400 for a request + * with no matching `loader`, 405 for no matching `action` or an unsupported + * method. + */ +const EXPECTED_REACT_ROUTER_ERROR_STATUSES = [400, 405] + +/** + * React Router's message arrives either bare (`Error.message`) or stringified + * (`String(error)`, which is what `.data` holds on the document path). The + * latter is prefixed with the error's name, which breaks the anchored + * `^Invalid request method` pattern. Strip it before matching so the patterns + * can stay anchored to the message itself. + */ +function stripErrorNamePrefix(message: string): string { + return message.replace(/^\w*Error:\s*/, '') +} + +export function isExpectedReactRouterErrorMessage(message: string): boolean { + return EXPECTED_REACT_ROUTER_ERROR_PATTERNS.some((pattern) => + pattern.test(message), + ) +} + +/** + * The `Error` behind a route error, when React Router kept one. + * + * `ErrorResponseImpl.error` is marked private, but it is what React Router's + * own default `handleError` reads and it holds the real message and stack. + * Handing the wrapper to Sentry instead files a stackless issue grouped on a + * synthesised message. + */ +export function getRouteErrorCause(error: unknown): unknown { + if (!isRouteErrorResponse(error)) return error + return (error as { error?: unknown }).error ?? error +} + +/** + * Narrows a value thrown by a loader/action to the React Router "no handler for + * this request" errors. + * + * These arrive as an `ErrorResponse`, not an `Error`, so `error instanceof + * Error` is false and `error.message` is undefined — in production the message + * lives on `.data` as a string. Matching on the message alone would also mean + * any unrelated error whose text happened to contain one of these phrases got + * dropped, so the status and the `internal` flag are checked first: the + * messages above are only ever produced alongside one of these two statuses, on + * an error React Router raised itself. + */ +export function isExpectedReactRouterRouteError(error: unknown): boolean { + if (!isRouteErrorResponse(error)) return false + // `internal: true` is how React Router marks the errors it generated itself + // (see `getInternalRouterError`). A route that throws its own 400/405 is + // marked `internal: false`, and that is a real bug worth reporting even if + // its text happens to contain one of the phrases below. + if ((error as { internal?: unknown }).internal !== true) return false + if (!EXPECTED_REACT_ROUTER_ERROR_STATUSES.includes(error.status)) return false + + // `.data` carries the message two ways. React Router wraps it in an `Error` + // (see `getInternalRouterError` in react-router's router.js) and unwraps it + // again when it renders a document, so the document path yields a + // stringified error; the single-fetch path yields the `Error`. Accept both. + const { data } = error + const message = + typeof data === 'string' + ? data + : data instanceof Error + ? data.message + : null + + return message + ? isExpectedReactRouterErrorMessage(stripErrorNamePrefix(message)) + : false +} + +type ErrorEventLike = { + message?: string + exception?: { values?: Array<{ value?: string | null } | null> | null } | null +} + +export function getEventErrorMessages(event: ErrorEventLike): string[] { + const messages: string[] = [] + for (const value of event.exception?.values ?? []) { + if (value?.value) messages.push(value.value) + } + if (event.message) messages.push(event.message) + return messages +} + +export function shouldDropErrorEvent(event: ErrorEventLike): boolean { + return getEventErrorMessages(event).some(isExpectedReactRouterErrorMessage) +} diff --git a/app/utils/timing.server.ts b/app/utils/timing.server.ts new file mode 100644 index 0000000..fd26a23 --- /dev/null +++ b/app/utils/timing.server.ts @@ -0,0 +1,92 @@ +export type Timings = Record< + string, + Array< + { desc?: string } & ( + { time: number; start?: never } | { time?: never; start: number } + ) + > +> + +export function makeTimings(type: string, desc?: string) { + const timings: Timings = { + [type]: [{ desc, start: performance.now() }], + } + Object.defineProperty(timings, 'toString', { + value: function () { + return getServerTimeHeader(timings) + }, + enumerable: false, + }) + return timings +} + +function createTimer(type: string, desc?: string) { + const start = performance.now() + return { + end(timings: Timings) { + let timingType = timings[type] + + if (!timingType) { + timingType = timings[type] = [] + } + timingType.push({ desc, time: performance.now() - start }) + }, + } +} + +export async function time( + fn: Promise | (() => ReturnType | Promise), + { + type, + desc, + timings, + }: { + type: string + desc?: string + timings?: Timings + }, +): Promise { + if (!timings) return typeof fn === 'function' ? fn() : fn + + // Start the timer before invoking `fn` and end it in a `finally`. A + // rejecting or throwing `fn` is exactly when a timing is most worth having, + // and it must not be the one case that drops the metric — so `fn()` has to + // be called inside the `try`, not hoisted above it. + const timer = createTimer(type, desc) + try { + return await (typeof fn === 'function' ? fn() : fn) + } finally { + timer.end(timings) + } +} + +export function getServerTimeHeader(timings?: Timings) { + if (!timings) return '' + return Object.entries(timings) + .map(([key, timingInfos]) => { + const dur = timingInfos + .reduce((acc, timingInfo) => { + const time = timingInfo.time ?? performance.now() - timingInfo.start + return acc + time + }, 0) + .toFixed(1) + const desc = timingInfos + .map((t) => t.desc) + .filter(Boolean) + .join(' & ') + return [ + key.replaceAll(/(:| |@|=|;|,|\/|\\)/g, '_'), + desc ? `desc=${JSON.stringify(desc)}` : null, + `dur=${dur}`, + ] + .filter(Boolean) + .join(';') + }) + .join(',') +} + +export function combineServerTimings(headers1: Headers, headers2: Headers) { + const newHeaders = new Headers(headers1) + newHeaders.append('Server-Timing', headers2.get('Server-Timing') ?? '') + return newHeaders.get('Server-Timing') ?? '' +} diff --git a/package-lock.json b/package-lock.json index 2924bf9..409bc22 100644 --- a/package-lock.json +++ b/package-lock.json @@ -24,7 +24,6 @@ "@tailwindcss/postcss": "^4.3.3", "@tailwindcss/typography": "^0.5.20", "address": "^2.0.3", - "chalk": "^5.6.2", "class-variance-authority": "^0.7.1", "close-with-grace": "^2.5.0", "clsx": "^2.1.1", diff --git a/package.json b/package.json index 6df2306..0b12708 100644 --- a/package.json +++ b/package.json @@ -62,7 +62,6 @@ "@tailwindcss/postcss": "^4.3.3", "@tailwindcss/typography": "^0.5.20", "address": "^2.0.3", - "chalk": "^5.6.2", "class-variance-authority": "^0.7.1", "close-with-grace": "^2.5.0", "clsx": "^2.1.1", diff --git a/react-router.config.ts b/react-router.config.ts index 888d000..4a7c512 100644 --- a/react-router.config.ts +++ b/react-router.config.ts @@ -1,10 +1,29 @@ import { type Config } from '@react-router/dev/config' export default { - // In v8 the future.v8_splitRouteModules flag moved here and defaults to - // true. v8_viteEnvironmentApi is gone because the Vite Environment API is - // now always on. - splitRouteModules: true, + // Defaults to true, but stated explicitly because it is load-bearing rather + // than decorative: @react-router/dev reads it to pick the Vite `appType` + // (its own plugin value wins over the one server/index.ts passes, and only + // resolves to 'spa' when ssr is false) and to force routeDiscovery to + // 'initial'. Setting this to false would flip both at once. + ssr: true, + + // The default is 'lazy' (fog of war), which ships only the matched routes + // and lets React Router discover the rest through a batched request that + // prefetches every on the page. 'initial' trades that away: the full + // route manifest is inlined into every HTML document instead of being a + // separate content-hashed, immutable asset cached once and reused. + // + // Worth it here because the manifest is 15 routes — a rounding error beside + // the document — so no navigation ever waits on a manifest request. Flip it + // back to 'lazy' if the route count grows or the HTML starts being + // edge-cached, since then the manifest is the thing that benefits from + // being cached on its own. + routeDiscovery: { mode: 'initial' }, + + // `future.v8_splitRouteModules` moved to the top level in v8 and defaults to + // true, so the explicit `splitRouteModules: true` is redundant. + // `v8_viteEnvironmentApi` is gone — the Vite Environment API is always on. future: { unstable_optimizeDeps: true, }, diff --git a/server/index.ts b/server/index.ts index 708c96b..a0ad85b 100644 --- a/server/index.ts +++ b/server/index.ts @@ -1,8 +1,8 @@ import crypto from 'node:crypto' +import { styleText } from 'node:util' import { createRequestHandler } from '@react-router/express' import * as Sentry from '@sentry/node' import { ip as ipAddress } from 'address' -import chalk from 'chalk' import closeWithGrace from 'close-with-grace' import compression from 'compression' import express from 'express' @@ -11,10 +11,12 @@ import getPort, { portNumbers } from 'get-port' import helmet from 'helmet' import morgan from 'morgan' import { type ServerBuild, RouterContextProvider } from 'react-router' +import { decodeRequestUrl } from './utils/request-url.js' const MODE = process.env.NODE_ENV ?? 'development' const IS_PROD = MODE === 'production' const IS_DEV = MODE === 'development' +const ALLOW_INDEXING = process.env.ALLOW_INDEXING !== 'false' const SENTRY_ENABLED = IS_PROD && process.env.SENTRY_DSN if (SENTRY_ENABLED) { @@ -26,6 +28,9 @@ const viteDevServer = IS_PROD : await import('vite').then((vite) => vite.createServer({ server: { middlewareMode: true }, + // We tell Vite we are running a custom app instead of + // the SPA default so it doesn't run HTML middleware + appType: 'custom', }), ) @@ -66,6 +71,13 @@ app.use(compression()) // http://expressjs.com/en/advanced/best-practice-security.html#at-a-minimum-disable-x-powered-by-header app.disable('x-powered-by') +if (!ALLOW_INDEXING) { + app.use((_, res, next) => { + res.set('X-Robots-Tag', 'noindex, nofollow') + next() + }) +} + if (viteDevServer) { app.use(viteDevServer.middlewares) } else { @@ -87,7 +99,12 @@ app.get(['/img/{*splat}', '/favicons/{*splat}'], (_req, res) => { return }) -morgan.token('url', (req) => decodeURIComponent(req.url ?? '')) +// Same fallback morgan's own `url` token uses, so mounting morgan at a path +// prefix cannot silently drop the prefix from every log line. morgan types the +// request as a bare IncomingMessage, but Express puts `originalUrl` on it. +morgan.token('url', (req) => + decodeRequestUrl((req as express.Request).originalUrl ?? req.url), +) app.use( morgan('tiny', { skip: (req, res) => @@ -234,7 +251,8 @@ if (!portAvailable && !IS_DEV) { const server = app.listen(portToUse, () => { if (!portAvailable) { console.warn( - chalk.yellow( + styleText( + 'yellow', `⚠️ Port ${desiredPort} is not available, using ${portToUse} instead.`, ), ) @@ -252,9 +270,9 @@ const server = app.listen(portToUse, () => { console.log( ` -${chalk.bold('Local:')} ${chalk.cyan(localUrl)} -${lanUrl ? `${chalk.bold('On Your Network:')} ${chalk.cyan(lanUrl)}` : ''} -${chalk.bold('Press Ctrl+C to stop')} +${styleText('bold', 'Local:')} ${styleText('cyan', localUrl)} +${lanUrl ? `${styleText('bold', 'On Your Network:')} ${styleText('cyan', lanUrl)}` : ''} +${styleText('bold', 'Press Ctrl+C to stop')} `.trim(), ) }) @@ -264,8 +282,8 @@ closeWithGrace(async ({ err }) => { server.close((e) => (e ? reject(e) : resolve('ok'))) }) if (err) { - console.error(chalk.red(err)) - console.error(chalk.red(err.stack)) + console.error(styleText('red', String(err))) + console.error(styleText('red', String(err.stack))) if (SENTRY_ENABLED) { Sentry.captureException(err) await Sentry.flush(500) diff --git a/server/utils/request-url.ts b/server/utils/request-url.ts new file mode 100644 index 0000000..8d45c00 --- /dev/null +++ b/server/utils/request-url.ts @@ -0,0 +1,16 @@ +/** + * Decodes a request URL for logging. + * + * morgan evaluates its format tokens from the response's `finished` event, so a + * token that throws does not fail a log line — it escapes as an uncaught + * exception and takes the process down mid-request. `decodeURIComponent` throws + * a `URIError` on a malformed percent-encoding, and scanners send paths like + * `/%` constantly, so fall back to the raw URL. + */ +export function decodeRequestUrl(url: string | undefined): string { + try { + return decodeURIComponent(url ?? '') + } catch { + return url ?? '' + } +} diff --git a/tests/allow-indexing.test.ts b/tests/allow-indexing.test.ts new file mode 100644 index 0000000..0a089e6 --- /dev/null +++ b/tests/allow-indexing.test.ts @@ -0,0 +1,165 @@ +import { spawn, type ChildProcess } from 'node:child_process' +import { existsSync } from 'node:fs' +import getPort, { portNumbers } from 'get-port' +import { afterAll, beforeAll, describe, expect, it } from 'vitest' + +/** + * `ALLOW_INDEXING` is read in two places that sit on either side of the + * app/server boundary: `server/index.ts` sets the `X-Robots-Tag` response + * header, and `app/root.tsx` renders a `` tag from the same + * value via `getEnv()`. Nothing in the type system links the two, so removing + * the key from `getEnv()` would leave the header behind and silently drop the + * tag while lint, typecheck, format and every other test stayed green. + * + * The flag is read once at process start, so each state needs its own process. + */ + +// Two different build outputs. The server runs the esbuild bundle +// (server-build/), which is what imports the React Router server build +// (build/server/) on demand. Only the second one is worth checking for +// existence, because that is what is missing on a tree that never built. +const REACT_ROUTER_BUILD = 'build/server/index.js' +const SERVER_BUNDLE = 'server-build/index.js' +const BOOT_TIMEOUT_MS = 30_000 + +/** + * NOTE: `ALLOW_INDEXING` is deliberately absent. `spawn` inherits + * `process.env`, so a developer who exports it in their shell would otherwise + * silently change what these tests assert; the two runs below set it + * explicitly. + */ +const BASE_ENV = { + NODE_ENV: 'production', + SESSION_SECRET: 'test-session-secret', + HONEYPOT_SECRET: 'test-honeypot-secret', + INTERNAL_COMMAND_TOKEN: 'test-command-token', + SENTRY_DSN: '', + NODEMAILER_HOST: 'test-host', + NODEMAILER_USER: 'test-user', + NODEMAILER_PASSWORD: 'test-password', + HASHNODE_PUBLICATION_ID: 'test-publication-id', + HASHNODE_PUBLICATION_HOST: 'test-publication-host', + POSTHOG_API_KEY: 'test-posthog-key', + UMAMI_WEBSITE_ID: 'test-umami-id', + UMAMI_DOMAINS: 'test-umami-domains', + UMAMI_DOMAIN: 'test-umami-domain', + UMAMI_SCRIPT_NAME: 'test-umami-script', + UMAMI_PUBLIC_ANALYTICS_URL: '', +} as const + +const servers: ChildProcess[] = [] + +/** Boots a server with `ALLOW_INDEXING` forced to `value` (or unset). */ +async function bootServer(allowIndexing: string | undefined) { + if (!existsSync(REACT_ROUTER_BUILD)) { + throw new Error( + `${REACT_ROUTER_BUILD} is missing. Run \`npm run build\` before this test; the deploy workflow's test job does this.`, + ) + } + + const port = await getPort({ port: portNumbers(51_000, 59_000) }) + const env = { ...process.env, ...BASE_ENV, PORT: String(port) } + if (allowIndexing === undefined) { + delete env.ALLOW_INDEXING + } else { + // `envSchema` types this as 'true' | 'false', and the point of the third + // run is a value the type forbids, so widen it here on purpose. + env.ALLOW_INDEXING = allowIndexing as NodeJS.ProcessEnv['ALLOW_INDEXING'] + } + + const child = spawn(process.execPath, [SERVER_BUNDLE], { + env, + stdio: ['ignore', 'pipe', 'pipe'], + }) + servers.push(child) + + let output = '' + child.stdout?.on('data', (chunk: Buffer) => (output += chunk.toString())) + child.stderr?.on('data', (chunk: Buffer) => (output += chunk.toString())) + + // /robots.txt is a resource route whose loader only reads request headers, + // so it proves the handler is mounted without reaching Hashnode. + const deadline = Date.now() + BOOT_TIMEOUT_MS + while (Date.now() < deadline) { + if (child.exitCode !== null) { + throw new Error(`server exited during startup\n${output}`) + } + try { + const response = await fetch(`http://127.0.0.1:${port}/robots.txt`, { + redirect: 'manual', + }) + if (response.status < 500) return `http://127.0.0.1:${port}` + } catch { + // not listening yet + } + await new Promise((resolve) => setTimeout(resolve, 250)) + } + + throw new Error(`server did not become ready\n${output}`) +} + +afterAll(() => { + for (const server of servers) server.kill('SIGKILL') +}) + +describe('ALLOW_INDEXING unset', () => { + let base = '' + + beforeAll(async () => { + base = await bootServer(undefined) + }, 60_000) + + it('is indexable: no header and no meta tag', async () => { + const response = await fetch(`${base}/talks`) + expect(response.status).toBe(200) + expect(response.headers.get('x-robots-tag')).toBeNull() + expect(await response.text()).not.toContain('name="robots"') + }) +}) + +describe('ALLOW_INDEXING=false', () => { + let base = '' + + beforeAll(async () => { + base = await bootServer('false') + }, 60_000) + + it('sends X-Robots-Tag: noindex, nofollow', async () => { + const response = await fetch(`${base}/talks`) + expect(response.headers.get('x-robots-tag')).toBe('noindex, nofollow') + }) + + it('renders the matching meta tag, so both boundaries agree', async () => { + const response = await fetch(`${base}/talks`) + expect(await response.text()).toContain( + ' { + const response = await fetch(`${base}/nope-does-not-exist`, { + redirect: 'manual', + }) + expect(response.status).toBe(404) + expect(response.headers.get('x-robots-tag')).toBe('noindex, nofollow') + }) +}) + +describe('ALLOW_INDEXING with a value the server does not recognise', () => { + // `server/index.ts` treats anything other than the exact string 'false' as + // "indexing allowed", and `envSchema` has to agree or a typo would fail + // validation inside `init()`. `init()` runs from the app bundle, which + // `getBuild()` imports per request behind a catch, so a disagreement here + // does not fail the boot — it 500s every route with no Sentry event. + let base = '' + + beforeAll(async () => { + base = await bootServer('') + }, 60_000) + + it('boots and serves instead of failing env validation', async () => { + const response = await fetch(`${base}/talks`) + expect(response.status).toBe(200) + expect(response.headers.get('x-robots-tag')).toBeNull() + }) +}) diff --git a/tests/request-url.test.ts b/tests/request-url.test.ts new file mode 100644 index 0000000..eaa89d9 --- /dev/null +++ b/tests/request-url.test.ts @@ -0,0 +1,21 @@ +import { describe, expect, it } from 'vitest' +import { decodeRequestUrl } from '../server/utils/request-url' + +describe('decodeRequestUrl', () => { + it('decodes a well-formed URL', () => { + expect(decodeRequestUrl('/talks?q=a%20b')).toBe('/talks?q=a b') + }) + + it('falls back to the raw URL when decoding throws', () => { + // morgan evaluates its tokens from the response's `finished` event, so a + // throw here escapes as an uncaught exception and takes the process down + // rather than failing a single log line. + expect(decodeRequestUrl('/%')).toBe('/%') + expect(decodeRequestUrl('/%zz')).toBe('/%zz') + expect(decodeRequestUrl('/%e0%a4%a')).toBe('/%e0%a4%a') + }) + + it('treats a missing URL as empty rather than throwing', () => { + expect(decodeRequestUrl(undefined)).toBe('') + }) +}) diff --git a/tests/sentry-event-filters.test.ts b/tests/sentry-event-filters.test.ts new file mode 100644 index 0000000..4c27eca --- /dev/null +++ b/tests/sentry-event-filters.test.ts @@ -0,0 +1,245 @@ +import { UNSAFE_ErrorResponseImpl as ErrorResponseImpl } from 'react-router' +import { describe, expect, it } from 'vitest' +import { + EXPECTED_REACT_ROUTER_ERROR_PATTERNS, + getEventErrorMessages, + getRouteErrorCause, + isExpectedReactRouterErrorMessage, + isExpectedReactRouterRouteError, + shouldDropErrorEvent, +} from '#app/utils/sentry-event-filters' + +const NO_ACTION = + 'You made a POST request to "/robots.txt" but did not provide an `action` for route "routes/_seo+/robots[.]txt", so there is no way to handle the request.' +/** + * Captured from a running production server. React Router stringifies the + * internal `Error` into `.data` on the document path, so the message arrives + * with an `Error: ` prefix — which is what silently broke the anchored + * `^Invalid request method` pattern. + */ +const NO_ACTION_STRINGIFIED = `Error: ${NO_ACTION}` +const INVALID_METHOD = 'Invalid request method "OPTIONS"' +const INVALID_METHOD_STRINGIFIED = `Error: ${INVALID_METHOD}` +const NO_LOADER = + 'You made a GET request to "/resources/theme-switch" but did not provide a `loader` for route "routes/resources/theme-switch", so there is no way to handle the request.' + +describe('isExpectedReactRouterErrorMessage', () => { + it('matches a request with no matching action', () => { + expect(isExpectedReactRouterErrorMessage(NO_ACTION)).toBe(true) + }) + + it('matches a request with no matching loader', () => { + expect(isExpectedReactRouterErrorMessage(NO_LOADER)).toBe(true) + }) + + it('matches an unsupported request method', () => { + expect( + isExpectedReactRouterErrorMessage('Invalid request method "OPTIONS"'), + ).toBe(true) + }) + + it('does not match an unrelated error', () => { + expect( + isExpectedReactRouterErrorMessage('Database connection failed'), + ).toBe(false) + expect(isExpectedReactRouterErrorMessage('')).toBe(false) + expect( + isExpectedReactRouterErrorMessage('Error: getServerSideProps failed'), + ).toBe(false) + }) + + // The `action`/`loader` patterns are unanchored, which is what lets them + // survive React Router rewording the surrounding text between versions. The + // cost is that a genuine bug whose message happens to contain the phrase is + // dropped too. That is inherited from upstream deliberately: React Router's + // own message is not part of its public API, so anchoring is not an option. + it('matches unanchored, so a real error containing the phrase is dropped too', () => { + expect( + isExpectedReactRouterErrorMessage( + 'Newsletter route did not provide an `action` handler', + ), + ).toBe(true) + }) + + it('exposes its patterns for callers that want to inspect them', () => { + expect(EXPECTED_REACT_ROUTER_ERROR_PATTERNS).toHaveLength(3) + }) +}) + +describe('getEventErrorMessages', () => { + it('collects the top-level message', () => { + expect(getEventErrorMessages({ message: 'boom' })).toEqual(['boom']) + }) + + it('collects every exception value, skipping empty ones', () => { + expect( + getEventErrorMessages({ + exception: { values: [{ value: 'first' }, { value: null }, {}] }, + }), + ).toEqual(['first']) + }) + + it('returns the exception values before the message', () => { + expect( + getEventErrorMessages({ + message: 'top', + exception: { values: [{ value: 'nested' }] }, + }), + ).toEqual(['nested', 'top']) + }) + + it('tolerates a missing or null exception', () => { + expect(getEventErrorMessages({})).toEqual([]) + expect(getEventErrorMessages({ exception: null })).toEqual([]) + expect(getEventErrorMessages({ exception: { values: null } })).toEqual([]) + }) +}) + +describe('shouldDropErrorEvent', () => { + it('drops an event whose exception value is an expected router error', () => { + expect( + shouldDropErrorEvent({ exception: { values: [{ value: NO_ACTION }] } }), + ).toBe(true) + }) + + it('drops an event whose message is an expected router error', () => { + expect( + shouldDropErrorEvent({ message: 'Invalid request method "TRACE"' }), + ).toBe(true) + }) + + it('keeps an event that is a real failure', () => { + expect( + shouldDropErrorEvent({ + exception: { values: [{ value: 'Unexpected server failure' }] }, + }), + ).toBe(false) + expect(shouldDropErrorEvent({})).toBe(false) + }) +}) + +describe('isExpectedReactRouterRouteError', () => { + // React Router reports these as an `ErrorResponse`, not an `Error`, so + // `error instanceof Error` is false and `error.message` is undefined. A + // filter written against `instanceof Error` silently never fires. + const routeError = (status: number, data: unknown, internal = true) => + new ErrorResponseImpl(status, 'status', data, internal) + + it('matches a 405 with no matching action', () => { + expect(isExpectedReactRouterRouteError(routeError(405, NO_ACTION))).toBe( + true, + ) + }) + + it('matches a 400 with no matching loader', () => { + expect(isExpectedReactRouterRouteError(routeError(400, NO_LOADER))).toBe( + true, + ) + }) + + it('matches an unsupported method', () => { + expect( + isExpectedReactRouterRouteError(routeError(405, INVALID_METHOD)), + ).toBe(true) + }) + + it('matches a stringified message, prefix and all', () => { + // This is the shape the document path actually produces. The anchored + // pattern does not match the `Error: ` prefix, so without stripping it + // these scanner requests keep paging. + expect( + isExpectedReactRouterRouteError( + routeError(405, INVALID_METHOD_STRINGIFIED), + ), + ).toBe(true) + expect( + isExpectedReactRouterRouteError(routeError(405, NO_ACTION_STRINGIFIED)), + ).toBe(true) + }) + + it('strips a custom error name prefix too', () => { + expect( + isExpectedReactRouterRouteError( + routeError(405, `TypeError: ${INVALID_METHOD}`), + ), + ).toBe(true) + }) + + it('matches when data is an Error, as it is in development', () => { + expect( + isExpectedReactRouterRouteError(routeError(405, new Error(NO_ACTION))), + ).toBe(true) + }) + + it('does not match a route-thrown error response', () => { + // React Router marks the errors it raises itself `internal: true` and a + // route's own 400/405 `internal: false`. A route throwing its own error + // is a real bug even when the text contains one of these phrases. + expect( + isExpectedReactRouterRouteError(routeError(405, NO_ACTION, false)), + ).toBe(false) + expect( + isExpectedReactRouterRouteError( + routeError(405, INVALID_METHOD_STRINGIFIED, false), + ), + ).toBe(false) + expect( + isExpectedReactRouterRouteError(routeError(400, NO_LOADER, false)), + ).toBe(false) + }) + + it('does not match a route error with another status', () => { + expect(isExpectedReactRouterRouteError(routeError(404, NO_ACTION))).toBe( + false, + ) + expect(isExpectedReactRouterRouteError(routeError(500, NO_ACTION))).toBe( + false, + ) + }) + + it('does not match a real error whose text contains the phrase', () => { + // The patterns are unanchored, so the status check is what keeps a + // genuine failure reportable. + expect( + isExpectedReactRouterRouteError( + new Error('newsletter route did not provide an `action` handler'), + ), + ).toBe(false) + expect(isExpectedReactRouterRouteError(routeError(500, NO_ACTION))).toBe( + false, + ) + }) + + it('does not match values that are not route errors', () => { + expect(isExpectedReactRouterRouteError(undefined)).toBe(false) + expect(isExpectedReactRouterRouteError(null)).toBe(false) + expect(isExpectedReactRouterRouteError(NO_ACTION)).toBe(false) + expect(isExpectedReactRouterRouteError({ status: 405 })).toBe(false) + expect(isExpectedReactRouterRouteError(routeError(405, undefined))).toBe( + false, + ) + }) +}) + +describe('getRouteErrorCause', () => { + it('returns the wrapped Error behind a route error', () => { + // Without this, `handleError` hands Sentry an `ErrorResponse` wrapper, + // which Sentry files as a stackless issue. + const cause = new Error(NO_ACTION) + const routeError = new ErrorResponseImpl(405, 'status', cause, true) + + expect(getRouteErrorCause(routeError)).toBe(cause) + }) + + it('falls back to the route error when it carries no cause', () => { + const routeError = new ErrorResponseImpl(404, 'Not Found', 'nope', true) + expect(getRouteErrorCause(routeError)).toBe(routeError) + }) + + it('passes a plain value straight through', () => { + const error = new Error('boom') + expect(getRouteErrorCause(error)).toBe(error) + expect(getRouteErrorCause('boom')).toBe('boom') + expect(getRouteErrorCause(undefined)).toBeUndefined() + }) +}) diff --git a/tests/server.smoke.test.ts b/tests/server.smoke.test.ts index 7245b9b..a2dfa8f 100644 --- a/tests/server.smoke.test.ts +++ b/tests/server.smoke.test.ts @@ -106,8 +106,14 @@ beforeAll(async () => { port = await getPort({ port: portNumbers(41_000, 49_000) }) base = `http://127.0.0.1:${port}` + const env = { ...process.env, ...TEST_ENV, PORT: String(port) } + // `spawn` inherits `process.env`, and TEST_ENV does not mention + // ALLOW_INDEXING. A developer who exports it would otherwise make the + // "sends no X-Robots-Tag by default" test below fail on their machine only. + delete env.ALLOW_INDEXING + const child = spawn(process.execPath, ['server-build/index.js'], { - env: { ...process.env, ...TEST_ENV, PORT: String(port) }, + env, stdio: ['ignore', 'pipe', 'pipe'], }) server = child @@ -154,6 +160,21 @@ describe('production server', () => { expect(response.status).toBe(200) }) + // React Router short-circuits these before any loader runs, so the root + // loader never produced data. The document still has to render the error + // boundary, and `useRequestInfo` used to assert the data was there — which + // turned a 405 into a 500 and buried the real cause. Scanners send these + // constantly. + it('answers an unsupported method with 405, not 500', async () => { + for (const method of ['POST', 'PUT', 'DELETE', 'PATCH', 'OPTIONS']) { + const response = await fetch(`${base}/talks`, { + method, + redirect: 'manual', + }) + expect(response.status, `${method} /talks`).toBe(405) + } + }) + it('404s an unknown path rather than serving the app shell', async () => { const response = await fetch(`${base}/nope-does-not-exist`) expect(response.status).toBe(404) @@ -249,4 +270,52 @@ describe('production server', () => { const response = await fetch(`${base}/talks`) expect(response.headers.get('ratelimit-limit')).toBeTruthy() }) + + it('reports Server-Timing for both the root loader and the render', async () => { + // The root loader's metric only reaches the client because + // `app/root.tsx` exports a `headers` function. React Router ignores a + // loader's response headers on the document without one, so dropping + // that export silently costs half the instrumentation and nothing else + // would notice. + const response = await fetch(`${base}/talks`) + const serverTiming = response.headers.get('server-timing') ?? '' + + expect(serverTiming).toContain('root_loader') + expect(serverTiming).toContain('render') + }) + + it('sends no X-Robots-Tag by default', async () => { + // ALLOW_INDEXING is unset for this server. The noindex behaviour is + // covered in tests/allow-indexing.test.ts, which boots its own process. + const response = await fetch(`${base}/talks`) + expect(response.headers.get('x-robots-tag')).toBeNull() + expect(await response.text()).not.toContain('name="robots"') + }) + + // The morgan `url` token decodes the whole request URL, query string + // included. morgan evaluates its tokens from the response's `finished` + // event with no try/catch, so a URIError there escapes as an uncaught + // exception and the process exits mid-request. Verified against `main`: + // this request kills the server, and the guard in + // `server/utils/request-url.ts` is what keeps it alive. + // + // The malformed value has to be in the query. A malformed *path* is thrown + // on by Express's own `decodeParam` before it reaches morgan, which returns + // a 400 and never evaluates the token, so that path cannot exercise this. + it('survives a query string with a malformed percent-encoding', async () => { + const response = await fetch(`${base}/talks?x=%`, { redirect: 'manual' }) + expect(response.status).toBe(200) + + // The uncaught throw lands on the `finished` event, which races the + // client's response resolving, so give the process a moment to die + // before asserting it is still up. + await new Promise((resolve) => setTimeout(resolve, 500)) + expect( + server?.exitCode, + `server should still be running${serverOutput ? `\n${serverOutput}` : ''}`, + ).toBeNull() + + const after = await fetch(`${base}/talks`) + expect(after.status).toBe(200) + }) }) diff --git a/tests/timing.test.ts b/tests/timing.test.ts new file mode 100644 index 0000000..6e9adab --- /dev/null +++ b/tests/timing.test.ts @@ -0,0 +1,112 @@ +import { describe, expect, it } from 'vitest' +import { + combineServerTimings, + getServerTimeHeader, + makeTimings, + time, +} from '#app/utils/timing.server' + +describe('getServerTimeHeader', () => { + it('returns an empty string with no timings', () => { + expect(getServerTimeHeader()).toBe('') + }) + + it('sanitises characters that are not allowed in a metric name', () => { + // The Server-Timing grammar does not allow `:`, `/`, `,` and friends in + // a metric name, so they are replaced with underscores. + const header = getServerTimeHeader({ + 'cache:get/user,1': [{ time: 1 }], + }) + expect(header).toBe('cache_get_user_1;dur=1.0') + }) +}) + +describe('makeTimings', () => { + it('serialises the open timer to a duration measured from creation', () => { + const timings = makeTimings('root loader', 'root loader') + const header = timings.toString() + + expect(header).toMatch(/^root_loader;desc="root loader";dur=\d+\.\d$/) + }) + + it('does not expose toString as an enumerable key', () => { + // The timings object is spread into nothing, but if it were ever logged + // or serialised an enumerable toString would show up as noise. + const timings = makeTimings('root loader') + expect(Object.keys(timings)).toEqual(['root loader']) + }) +}) + +describe('time', () => { + it('records a duration for the timed work', async () => { + const timings = makeTimings('root loader') + + const result = await time(async () => 'value', { + timings, + type: 'work', + desc: 'the work', + }) + + expect(result).toBe('value') + expect(getServerTimeHeader(timings)).toContain('work;desc="the work";dur=') + }) + + it('accepts an already-started promise', async () => { + const timings = makeTimings('root loader') + await expect( + time(Promise.resolve(1), { timings, type: 'work' }), + ).resolves.toBe(1) + expect(getServerTimeHeader(timings)).toContain('work;dur=') + }) + + it('resolves without timings and does not throw', async () => { + await expect(time(async () => 2, { type: 'work' })).resolves.toBe(2) + }) + + it('still records a duration when the timed work rejects', async () => { + // A failing loader is exactly when a timing is most worth having, so it + // must not be the one case that drops the metric. + const timings = makeTimings('root loader') + + await expect( + time( + async () => { + throw new Error('boom') + }, + { timings, type: 'work', desc: 'the work' }, + ), + ).rejects.toThrow('boom') + + expect(getServerTimeHeader(timings)).toContain('work;desc="the work";dur=') + }) + + it('records a duration when the work throws synchronously', async () => { + const timings = makeTimings('root loader') + + await expect( + time( + () => { + throw new Error('boom') + }, + { timings, type: 'work' }, + ), + ).rejects.toThrow('boom') + + expect(getServerTimeHeader(timings)).toContain('work;dur=') + }) +}) + +describe('combineServerTimings', () => { + // `Headers.append` joins repeated values with ", " per the Fetch spec. + it('appends the second header to the first', () => { + const headers1 = new Headers({ 'Server-Timing': 'a;dur=1' }) + const headers2 = new Headers({ 'Server-Timing': 'b;dur=2' }) + + expect(combineServerTimings(headers1, headers2)).toBe('a;dur=1, b;dur=2') + }) + + it('appends an empty entry when the second header is absent', () => { + const headers1 = new Headers({ 'Server-Timing': 'a;dur=1' }) + expect(combineServerTimings(headers1, new Headers())).toBe('a;dur=1, ') + }) +})