Render Contentful assets #279
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Benaiah
changed the title
src/hooks.server.ts
Outdated
| : createConnectedRedisClient({ url: KV_URL, useTLS: !KV_URL.startsWith("redis://localhost") }); | ||
|
|
||
| // So we don't get unhandled promise rejection warnings if nothing awaits the promise | ||
| redisClientPromise.catch((err) => console.error(err)); |
There was a problem hiding this comment.
If #274 is merged first, this will need to be updated to use the logger added in that PR.
Benaiah
force-pushed
the
benaiah/render-contentful-references-and-assets
branch
from
37c1ee4 to
931622e
Compare
| if (!item?.sys?.id || !item?.url || !item?.contentType?.startsWith("image/")) return []; | ||
| const blurhashResponse = await fetch(`/api/v1/blurhash/${encodeURIComponent(item.url)}`); | ||
| if (!blurhashResponse.ok) return []; | ||
| return [item.sys.id, await blurhashResponse.text()]; |
There was a problem hiding this comment.
| return [item.sys.id, await blurhashResponse.text()]; | |
| return [[item.sys.id, await blurhashResponse.text()]]; |
| const errorMessage = getErrorMessage(body); | ||
| if (errorMessage) return errorMessage; |
There was a problem hiding this comment.
getErrorMessage always returns a value so getDataMessage is not being used right now.
src/lib/components/ContentfulRichText/nodes/AssetHyperlink.svelte
Outdated
Show resolved
Hide resolved
Benaiah
force-pushed
the
benaiah/render-contentful-references-and-assets
branch
from
e7180ed to
e6365ff
Compare
|
@Benaiah the |
Benaiah
force-pushed
the
benaiah/render-contentful-references-and-assets
branch
from
e7e0039 to
2b38b88
Compare
|
Giving this a look now; things seemed rather slow on first run-through but then I changed some I'm still seeing the I think the only blocker might be that the image on this page seems to run into an issue where it fails to load, although I haven't looked into why yet: https://ldaf-git-benaiah-render-contentful-references-and-assets-ldaf.vercel.app/plants/pests |
| let redisClientError: undefined | unknown; | ||
| const redisClient = createRedisClient<None, None, None>({ | ||
| url, | ||
| socket: { tls: useTLS }, |
There was a problem hiding this comment.
I think we may be able to fix the socket connection error with the pingInterval config, although I'm not sure if this could have side effects outside of local development (so we may only want to add this config for local dev). After making this change locally, I didn't see the error pop up after running dev for over 10 minutes (usually I saw the error pop up around the 5 minute mark).
| socket: { tls: useTLS }, | |
| socket: { tls: useTLS }, | |
| // ping every 5 minutes to avoid a socket connection error in local development | |
| pingInterval: 5 * 60 * 1000, | |
| }); |
Found the suggestion in this issue comment, and the docs for the createClient configs are here.
@Benaiah @hinzed1127 let me know if this solves the issue for y'all locally!
There was a problem hiding this comment.
it seemed to make it last longer (maybe 10 minutes?), but I was still having things crash after a period of time
There was a problem hiding this comment.
Got a question and a minor comment, but otherwise this LGTM! Only blocker is figuring out why we're getting an error on the image on the /plants/pests page.
|
|
||
| export const handle = (async ({ event, resolve }) => { | ||
| const handleSetupRedisClient = (async ({ event, resolve }) => { |
There was a problem hiding this comment.
I'm a little confused about how this works. Server hooks get run when the app starts and then on every request right? Are we spinning up a new Redis client for every request? Is it possible for us to check whether event.locals.getKVClient has already been set so that we don't create a new client or reset it?
There was a problem hiding this comment.
When running on Vercel (as opposed to local development) there's no real distinction between "when the app starts" and "when it handles a request" because we're using serverless functions, so the app starts and runs to completion for each request. We can't persist a client between different invocations of a serverless function.
| const pageMetadata = pageMetadataMap.get(metadataID); | ||
| if (!pageMetadata) break fetchData; |
There was a problem hiding this comment.
If we're going to break here then this should probably be run before we request the data right?
| type ServiceGroup = ExtractQueryType< | ||
| ServiceGroupCollectionQuery, | ||
| ["serviceGroupCollection", "items", number] | ||
| >; | ||
|
|
||
| if (matchedServiceGroup) { | ||
| const serviceEntries = matchedServiceGroup.serviceEntriesCollection?.items.filter( | ||
| (item) => item?.__typename === "ServiceEntry" | ||
| ); | ||
| let serviceGroups = matchedServiceGroup.serviceEntriesCollection?.items.filter( | ||
| (item) => item?.__typename === "ServiceGroup" | ||
| ) as ServiceGroup[]; | ||
| type ServiceGroupMetadata = ExtractQueryType<ServiceGroup, ["pageMetadata"]>; | ||
|
|
||
| serviceGroups = serviceGroups.map((serviceGroup) => { | ||
| const serviceGroupMetadata = pageMetadataMap.get( | ||
| serviceGroup?.pageMetadata?.sys.id || "" | ||
| ); | ||
| return { ...serviceGroup, url: serviceGroupMetadata?.url }; | ||
| }); | ||
| type ChildServiceEntryOrGroup = ExtractQueryType< | ||
| ServiceGroup, | ||
| ["serviceEntriesCollection", "items", number] | ||
| >; | ||
|
|
||
| return { | ||
| ...matchedServiceGroup, | ||
| pageMetadata: matchedPageMetadata, | ||
| serviceEntries, | ||
| serviceGroups, | ||
| } as ServiceGroupPage; | ||
| } | ||
| } else { | ||
| console.warn( | ||
| `A Service Group entry with the slug "${slug}" could not be found. If this page was reached via a link, it is likely that the Page Metadata entry is published but the Service Group entry is not.` | ||
| ); | ||
| } | ||
| type ChildServiceEntry = Extract<ChildServiceEntryOrGroup, { __typename: "ServiceEntry" }>; | ||
|
|
||
| type ChildServiceGroup = Extract<ChildServiceEntryOrGroup, { __typename: "ServiceGroup" }>; | ||
|
|
||
| export type ServiceGroupPage = { | ||
| serviceGroup: ServiceGroup & { | ||
| heroImage?: ServiceGroup["heroImage"] & { | ||
| imageSource?: NonNullable<ServiceGroup["heroImage"]>["imageSource"] & { | ||
| blurhash?: string | null | undefined; | ||
| }; | ||
| }; | ||
| description?: ServiceGroup["description"] & { | ||
| blurhashes?: Record<string, string> | null | undefined; | ||
| }; | ||
| }; | ||
| childServiceEntries: (ChildServiceEntry & { | ||
| description?: ChildServiceEntry["description"] & { | ||
| blurhashes?: Record<string, string> | null | undefined; | ||
| }; | ||
| })[]; | ||
| childServiceGroups: (ChildServiceGroup & { url?: string | null | undefined })[]; | ||
| pageMetadata?: ServiceGroupMetadata; | ||
| }; |
There was a problem hiding this comment.
Would it make sense to export all these types from a src/routes/(infoPages)/[topTierPage]/[...serviceGroupPage]/types.ts file and import them here?
| } | ||
| serviceEntriesCollection(limit: 10) { | ||
| serviceEntriesCollection(limit: 5) { |
There was a problem hiding this comment.
You might have been hitting query complexity limits, but this'll be too small for some service entries. E.g. /animals/meat-poultry currently has 8 service entries, so we'll probably want at least that. Ideally figure out with UX what the ceiling on number of service entries might be (10? higher?)
|
Coverage after merging benaiah/render-contentful-references-and-assets into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Reports for f75d45d have been deployed to Vercel: |
There was a problem hiding this comment.
LGTM after a quick click-test.
I'm not seeing blurhashes when navigating between Core Content pages that both have hero images (the previous hero stays in place until the new one loads, instead of showing the blurhash first), but we can handle that in a follow-up.
Jira ticket: LDAF-248
Proposed changes
/api/v1/blurhashroute that creates blurhashes on-demand from Contentful images and caches them in RedisScreenshots
Acceptance criteria validation
Other details
Alternate solutions
We considered using Imgix for blurhashes, and we could still easily switch to that, but the API endpoint included here was a small lift and means we don't have to worry about the number of images we have or go thru procurement for anything.