-
Notifications
You must be signed in to change notification settings - Fork 2
Add Support for CMP DAM assets #180
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…for ContentReference handling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
exacs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| alt( | ||
| 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 url; | ||
|
|
||
| return fallback ?? ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this alt function should not belong here (getPreviewUtils) because it has nothing to do with preview
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll move this outside of getPreviewUtils.
| srcset(input: InferredContentReference | null | undefined): string { | ||
| if (!input?.item || !('Renditions' in input.item)) return ''; | ||
|
|
||
| const renditions = input.item.Renditions; | ||
| if (!renditions || renditions.length === 0) return ''; | ||
|
|
||
| // Track seen widths to avoid duplicate width descriptors | ||
| const seenWidths = new Set<number>(); | ||
|
|
||
| const srcsetEntries = renditions | ||
| .filter((r) => { | ||
| if (!r.Url || !r.Width) return false; | ||
| // Skip if we've already seen this width | ||
| if (seenWidths.has(r.Width)) return false; | ||
| seenWidths.add(r.Width); | ||
| return true; | ||
| }) | ||
| .map((r) => { | ||
| const url = appendPreviewToken(r.Url!); | ||
| return `${url} ${r.Width}w`; | ||
| }); | ||
|
|
||
| return srcsetEntries.join(', '); | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this function does too many things. If this is under getPreviewUtils, it should not convert to the HTML attribute srcset. I think is better if this returns InferredContentReference with the preview token appended for each URL and nothing more.
And then, separately, outside getPreviewUtils, we can implement something that converts InferredContentReference to srcset
Actually, if the function only appends preview tokens to URLs, we don't need two separate functions (src and srcset). I think is more interesting to have one function that does that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like i missed a part this function should be able fetch a particular rendition. But anyway I will move it outside
| src( | ||
| input: string | InferredContentReference | null | undefined, | ||
| options?: { renditionName?: string } | ||
| ): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also have concerns here. Since this src function is inside getPreviewUtils, I think the function should be much lower level and only append preview tokens to image URLs
I mean:
- if
inputis astring, it returns the url with the preview token as string - if
inputisInferredContentReference, returnsInferredContentReferencewhere each url has the preview token
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we return InferredContentReference then a user will have to handle a lot of stuff like validations and guards. I'll create something similar to getPreviewUtils which focus on damAssets . It should reduce number of steps to get srcset, alt.
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.