🔨 incoprorate PR feedback

functions/_common/env.ts

@@ -0,0 +1,12 @@
+export interface Env {
+    ASSETS: {
+        fetch: typeof fetch
+    }
+    url: URL
+    GRAPHER_CONFIG_R2_BUCKET_URL: string
+    GRAPHER_CONFIG_R2_BUCKET_FALLBACK_URL: string
+    GRAPHER_CONFIG_R2_BUCKET_PATH: string
+    GRAPHER_CONFIG_R2_BUCKET_FALLBACK_PATH: string
+    CF_PAGES_BRANCH: string
+    ENV: string
+}

functions/_common/grapherRenderer.ts

@@ -16,7 +16,7 @@ import LatoRegular from "../_common/fonts/LatoLatin-Regular.ttf.bin"
 import LatoMedium from "../_common/fonts/LatoLatin-Medium.ttf.bin"
 import LatoBold from "../_common/fonts/LatoLatin-Bold.ttf.bin"
 import PlayfairSemiBold from "../_common/fonts/PlayfairDisplayLatin-SemiBold.ttf.bin"
-import { Env } from "../grapher/thumbnail/[slug].js"
+import { Env } from "./env.js"
 
 declare global {
     // eslint-disable-next-line no-var
@@ -164,11 +164,11 @@ interface FetchGrapherConfigResult {
     etag: string | undefined
 }
 
-export async function fetchGrapherConfig(
+export async function fetchUnparsedGrapherConfig(
     slug: string,
     env: Env,
-    etag: string
-): Promise<FetchGrapherConfigResult> {
+    etag?: string
+) {
     // The top level directory is either the bucket path (should be set in dev environments and production)
     // or the branch name on preview staging environments
     console.log("branch", env.CF_PAGES_BRANCH)
@@ -205,16 +205,28 @@ export async function fetchGrapherConfig(
     }
 
     // Fetch grapher config
-    const fetchResponse = await fetchFromR2(requestUrl, undefined, fallbackUrl)
+    return fetchFromR2(requestUrl, etag, fallbackUrl)
+}
+
+export async function fetchGrapherConfig(
+    slug: string,
+    env: Env,
+    etag?: string
+): Promise<FetchGrapherConfigResult> {
+    const fetchResponse = await fetchUnparsedGrapherConfig(slug, env, etag)
 
     if (fetchResponse.status !== 200) {
-        console.log("Failed to fetch grapher config", fetchResponse.status)
+        console.log(
+            "Status code is not 200, returning empty response with status code",
+            fetchResponse.status
+        )
         return {
             grapherConfig: null,
             status: fetchResponse.status,
             etag: fetchResponse.headers.get("etag"),
         }
     }
+
     const grapherConfig: GrapherInterface = await fetchResponse.json()
     console.log("grapher title", grapherConfig.title)
     return {
@@ -232,7 +244,7 @@ async function fetchAndRenderGrapherToSvg(
 ) {
     const grapherLogger = new TimeLogger("grapher")
 
-    const grapherConfigResponse = await fetchGrapherConfig(slug, env, etag)
+    const grapherConfigResponse = await fetchGrapherConfig(slug, env)
 
     if (grapherConfigResponse.status !== 200) {
         return null

functions/grapher/[slug].ts

@@ -1,8 +1,8 @@
-import { Env } from "./thumbnail/[slug].js"
+import { Env } from "../_common/env.js"
 import {
-    fetchGrapherConfig,
     getOptionalRedirectForSlug,
     createRedirectResponse,
+    fetchUnparsedGrapherConfig,
 } from "../_common/grapherRenderer.js"
 import { IRequestStrict, Router, error, cors } from "itty-router"
 
@@ -141,7 +141,7 @@ async function handleConfigRequest(
     etag: string | undefined
 ) {
     const shouldCache = searchParams.get("nocache") === null
-    console.log("Preapring json response for ", slug)
+    console.log("Preparing json response for ", slug)
     // All our grapher slugs are lowercase by convention.
     // To allow incoming links that may contain uppercase characters to work, we redirect to the lowercase version.
     const lowerCaseSlug = slug.toLowerCase()
@@ -153,14 +153,15 @@ async function handleConfigRequest(
         )
 
         return createRedirectResponse(
-            redirectSlug ? `${redirectSlug}.config.json` : lowerCaseSlug,
+            `${redirectSlug ?? lowerCaseSlug}.config.json`,
             env.url
         )
     }
 
-    const grapherPageResp = await fetchGrapherConfig(slug, env, etag)
+    const grapherPageResp = await fetchUnparsedGrapherConfig(slug, env, etag)
 
     if (grapherPageResp.status === 304) {
+        console.log("Returning 304 for ", slug)
         return new Response(null, { status: 304 })
     }
 
@@ -173,27 +174,31 @@ async function handleConfigRequest(
             env
         )
         if (redirectSlug && redirectSlug !== slug) {
+            console.log("Redirecting to ", redirectSlug)
             return createRedirectResponse(
                 `${redirectSlug}.config.json`,
                 env.url
             )
         } else {
+            console.log("Returning 404 for ", slug)
             // Otherwise we just return the status code.
             return new Response(null, { status: grapherPageResp.status })
         }
     }
 
-    console.log("Grapher page response", grapherPageResp.grapherConfig.title)
+    console.log("Returning 200 for ", slug)
 
     const cacheControl = shouldCache
         ? "public, s-maxage=3600, max-age=0, must-revalidate"
         : "public, s-maxage=0, max-age=0, must-revalidate"
 
-    return new Response(JSON.stringify(grapherPageResp.grapherConfig), {
+    //grapherPageResp.headers.set("Cache-Control", cacheControl)
+    return new Response(grapherPageResp.body as any, {
+        status: 200,
         headers: {
-            "content-type": "application/json",
+            "Content-Type": "application/json",
             "Cache-Control": cacheControl,
-            ETag: grapherPageResp.etag,
+            ETag: grapherPageResp.headers.get("ETag") ?? "",
         },
     })
 }

functions/grapher/thumbnail/[slug].ts

@@ -1,19 +1,7 @@
+import { Env } from "../../_common/env.js"
 import { fetchAndRenderGrapher } from "../../_common/grapherRenderer.js"
 import { IRequestStrict, Router, error } from "itty-router"
 
-export interface Env {
-    ASSETS: {
-        fetch: typeof fetch
-    }
-    url: URL
-    GRAPHER_CONFIG_R2_BUCKET_URL: string
-    GRAPHER_CONFIG_R2_BUCKET_FALLBACK_URL: string
-    GRAPHER_CONFIG_R2_BUCKET_PATH: string
-    GRAPHER_CONFIG_R2_BUCKET_FALLBACK_PATH: string
-    CF_PAGES_BRANCH: string
-    ENV: string
-}
-
 const router = Router<IRequestStrict, [URL, Env, ExecutionContext]>()
 router
     .get(