Add Support for CMP DAM assets #180
Conversation
…for ContentReference handling
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive support for Content Management Platform (CMP) Digital Asset Management (DAM) assets to the Optimizely CMS SDK. It enables applications to fetch and utilize rich asset metadata including renditions, focal points, and tags from the GraphQL API when working with content references.
Key Changes
- Conditional DAM fragment generation: Introduces a
damEnabledflag to GraphClient that conditionally includes DAM-specific GraphQL fragments only when needed, optimizing query size for non-DAM scenarios - Enhanced type system: Adds strongly-typed asset models (PublicImageAsset, PublicVideoAsset, PublicRawFileAsset) with discriminated unions and updates InferredContentReference to include optional asset item data
- Helper utilities: Extends getPreviewUtils() with src(), srcset(), and alt() functions that intelligently extract URLs and metadata from DAM assets with automatic preview token handling and rendition selection
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
packages/optimizely-cms-sdk/src/model/assets.ts |
Defines TypeScript types for DAM assets (PublicImageAsset, PublicVideoAsset, PublicRawFileAsset) with renditions, tags, and metadata |
packages/optimizely-cms-sdk/src/infer.ts |
Updates ContentReferenceProperty inference to include optional item field with asset details |
packages/optimizely-cms-sdk/src/util/baseTypeUtil.ts |
Adds CONTENT_REFERENCE_ITEM_FRAGMENTS and updates buildBaseTypeFragments() to conditionally include DAM fragments |
packages/optimizely-cms-sdk/src/graph/createQuery.ts |
Propagates damEnabled flag through all query generation functions to conditionally include ContentReferenceItem fragments |
packages/optimizely-cms-sdk/src/graph/index.ts |
Adds damEnabled option to GraphClient constructor and passes it to query generation |
packages/optimizely-cms-sdk/src/react/server.tsx |
Enhances getPreviewUtils() with src(), srcset(), and alt() helpers supporting both legacy URLs and DAM assets |
packages/optimizely-cms-sdk/src/react/__test__/getPreviewUtils.test.tsx |
Comprehensive test coverage for new helper functions including rendition selection, deduplication, and preview token handling |
packages/optimizely-cms-sdk/src/graph/__test__/createQueryDAM.test.ts |
Tests DAM fragment generation across simple, nested, and array scenarios with damEnabled flag |
samples/nextjs-template/src/components/AboutUs.tsx |
Demonstrates DAM usage by replacing Next.js Image component with native img using src() and alt() helpers |
samples/nextjs-template/src/components/SmallFeature.tsx |
Updates src() call to use new ContentReference API (passes entire object instead of url.default) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…set, and alt methods
TRomesh
had a problem deploying
to
Vercel preview environment
GitHub Actions
Failure
exacs
left a comment
exacs
left a comment
There was a problem hiding this comment.
The implementation looks good! I have no objections in the GraphClient and infer part.
However, I think the src, alt and srcset are too high level, especially because they are under getPreviewUtils. I expect those functions to only append preview tokens, not converting Graph responses to HTML attributes.
If you want to implement high level functions that convert graph responses into HTML attributes, they are separate functions. You can also build a React component Image (like Next.js Image) where you pass the image object (opti.image) and renders the image, but it feels a separate PR.
TRomesh
had a problem deploying
to
Vercel preview environment
GitHub Actions
Failure
exacs
left a comment
exacs
left a comment
There was a problem hiding this comment.
This looks much better but I have concerns about null, undefined and how the functions handle those values.
| export function getAlt( | ||
| input: InferredContentReference | null | undefined, | ||
| fallback?: string | ||
| ): string { | ||
| if (!input) return fallback ?? ''; | ||
|
|
||
| // Check if item has AltText property (PublicImageAsset or PublicVideoAsset) | ||
| if (input.item && 'AltText' in input.item) { | ||
| const altText = input.item.AltText ?? ''; | ||
| return altText || (fallback ?? ''); | ||
| } | ||
|
|
||
| return fallback ?? ''; | ||
| } |
There was a problem hiding this comment.
I am not sure if getAlt method should return empty string if no fallback nor input is provided.
- On one hand,
altattribute is required and helpful from accessibility perspective - On the other hand
alt=""(empty string) has a particular meaning ("image is not key part of the page e.g. is decoration"). Should the SDK assume this?
| */ | ||
| export function getSrcset<T extends Record<string, any>>( | ||
| opti: T & { __context?: { preview_token?: string } }, | ||
| property: InferredContentReference | null | undefined |
There was a problem hiding this comment.
Is this correct?
If a developer passes null or undefined, this will return empty string resulting in:
<img srcset="" />
Is it correct?
DAM-enabled GraphQL queries: Content references now fetch asset details (images, videos, files) with renditions, focal points, and tags when damEnabled param is set to true.
Conditional fragment generation: DAM fragments only included when needed to optimize query size.
Enhanced ContentReferenceItem: Added BaseAsset type and properly typed PublicImageAsset, PublicVideoAsset, PublicRawFileAsset with discriminated unions.
Updated InferredContentReference: Now includes optional item property with asset details.
Added helper functions to getPreviewUtils(): Extracts URL from assets, supports rendition selection, auto-appends preview tokens.
src: Update method to take contentReference type object and extract url or Url from DAM.srcset: Generates responsive srcset from renditions with automatic width deduplication.alt: Extracts AltText from DAM assets.