Implement internal and external redirects using PageMetadata from Contentful. #557
Conversation
…v, prebuild, pretest, and precheck).
…in the base layout server file.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
src/routes/+layout.server.ts
Outdated
| pageMetadata.internalRedirect.sys.id, | ||
| ); | ||
| if (internalRedirectPageMetadata?.url) { | ||
| throw redirect(301, internalRedirectPageMetadata.url); |
There was a problem hiding this comment.
301 felt like the right option here for the ones available, but let me know if you think there's a better option.
There was a problem hiding this comment.
I think this is a totally viable solution. The only alternative approach I can immediately think of is having a separate metadata query that's much slimmer and only grabs redirects. Or in a similar vein, just making the includeRedirects and includeBreadcrumbs named arguments default to false in the function definition, as to avoid accruing too much bloat in the function arguments, but that concern might still be a little premature.
|
Got redirects to News and Events pages mostly working. Now noticing that my workaround to redirect from a News page has added a bunch of junk to the sidebar, which we don't want. Will look into this a bit more tomorrow. |
…ge default behavior to not needing redirects and breadcrumbs.
| id | ||
| } | ||
| } | ||
| internalRedirect { |
There was a problem hiding this comment.
RE: @hinzed1127 separating the query so we only grab redirects, I think that would have been less efficient since we would be making double the requests to Contentful (when we could just grab it all in one go while we're working on loading the PageMetadataMap).
That said, I wanted to create a fragment so we could have two separate queries (one for all the entries and one for only entries that don't include redirects), but in both cases here we're still asking for data on internalRedirect and externalRedirect even though in one of those cases we know that these will all come back null. Unsure how to clean this up so that we don't query for values we know are null without removing the fragment and duplicating the majority of the query.
There was a problem hiding this comment.
Unsure how to clean this up so that we don't query for values we know are null...
Not sure if I follow this bit. What scenario would do that?
There was a problem hiding this comment.
The fragment here is querying for internalRedirect and externalRedirect, but we use the fragment for both types of queries:
queryAll- we do this whenincludeRedirectsistrueand therefore we want the data oninternalRedirectandexternalRedirectqueryWithoutRedirects- we do this whenincludeRedirectsisfalseand we apply a filter:But because we're reusing the same fragment we'll still get backwhere: { AND: [{ internalRedirect_exists: false }, { externalRedirect_exists: false }] }internalRedirectandexternalRedirectin the response (all of which will benullbased on the filter).
Ideally we wouldn't query for internalRedirect and externalRedirect in the second case, but I don't think there's a way to do that without duplicating the GraphQL query?
| includeBreadcrumbs = false, | ||
| includeRedirects = false, |
There was a problem hiding this comment.
Changed these to default to false per your suggestion @hinzed1127. Even though the main use-case includes these, we have more use-cases where we don't.
|
Coverage after merging louis/redirects into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Reports for 9dcc256 have been deployed to Vercel: |
We had originally built the
Page detailscontent type with support for internal redirects (a reference to anotherPage detailsentry) and external redirects (a URL string), but we never implemented the feature on the site.Proposed changes
pageMetadataMapbased on the need for redirects. Change our default parameters forloadPageMetadataMapso that we assume we don't need breadcrumbs or redirects. Include the data for redirects in the query, if available.src/routes/+layout.server.ts), we attempt to find a matchingPage detailsentry for the requested path, check if that entry has any internal or external redirects, andthrowan error using the built-in SvelteKitredirectmethod.Page detailsso that (in addition to anotherPage detailsentry) it will also accept aNewsorEvententry. Support all three types of internal redirects in the base layout server file.Acceptance criteria validation
To test this out locally, switch your
DEFAULT_CONTENTFUL_ENVIRONMENTin your.env.localconfig file frommastertoredirect-dev(I set up this env in Contentful to avoid causing any problems on production)./indian-creek-recreation-area/book-your-stay-todayexternally redirects to the ResNexus sitePage detailsentry: Book your stay at Indian Creek (This entry was already set up with an external redirect, so no changes were made here.)/indian-creek-recreation-area/getting-to-indian-creekinternally redirects to/indian-creek-recreation-area/activities-amenities.Page detailsentry: Getting to Indian Creek (This is not a real use-case and was just added for testing.)/online-paymentsinternally redirects to/business/online-payments.Page detailsentry: Horticulture Online Payments (This entry was already set up with an internal redirect, so I just set the parent page to the homepage for testing.)/indian-creek-recreation-area/campground-mapsinternally redirects to/about/events/event/2024-05-29-louisiana-agricultural-commodities-commission-meeting.Page detailsentry: Indian Creek Maps (For testing only.)/indian-creek-recreation-area/pavilion-reservationsinternally redirects to/about/news/article/ag-commissioner-mike-strain-in-cuba-building-bridges-for-la-export.Page detailsentry: Pavilion reservations (For testing only.)Other details
Alternate solutions
While it might have been nice to handle this in a
hookinstead, I wanted to keep things relatively simple and add the logic after we load thepageMetadataMap(I'm honestly not sure how this would look in ahookunless we built the map in the hook, which doesn't seem right.).I originally added some of this logic in the individual
+page.server.tsfiles for the[topTierPage]and[...serviceGroupPage]routes, but this was repetitive, and I think handling it in the base layout is cleaner. We are doing a bit of double work now that we're looking up thePage detailsentry via the map twice (once in the base layout to determine a redirect and once again when loading Top Tier and Core Content pages), so this could probably still be improved.Possible drawbacks
This setup allows for redirecting from any path (both those that already exist and ones that don't exist yet), but the editor experience for redirecting from a non-existent path (especially a nested one) is a bit weird. For instance, if you wanted to redirect from a new nested path
/abc/xyzto/, you would need to set up entries for bothabcandxyzand add the relevant redirect onxyz. Ifabcis just there for the URL, you will need to add a redirect onabcas well so that it doesn't show up as a weird error page.Additionally, there doesn't seem to be an easy way to prevent an infinite redirect loop; if two pages redirect to each other, the site will likely break pretty badly. We could add some basic protection against this in the base layout server file, but it would decrease efficiency and still be relatively difficult to handle complex loop situations (e.g.
/aredirects to/b,/bredirects to/c, and/credirects to/a). Rather than trying to protect against this edge case, it's probably wiser to alert editors to this edge case in the user guide.Requested feedback
I'll leave a self-review, but I'm rusty, so any and all feedback here is appreciated!
To-Do
Some things we should probably think through / update:
pageMetadataMapin both the base layout and individual page server files (it's only loaded from Contentful once, but the extra lookups are wasteful).TODOcomment in the base layout server file.Page detailsentries such as news and events.Before merging / deploying, we'll want to:
redirect-devenvironment./online-paymentsto internally redirect to/business/online-paymentsand/indian-creek-recreation-area/book-your-stay-todayto externally redirect to ResNexus).