refactor: Modularize AEM Asset Selector into testable helpers with explicit repository mode support#841
Conversation
| */ | ||
| export function buildDmUrl(asset, dmOrigin) { | ||
| const mimetype = asset.mimetype || asset['dc:format'] || ''; | ||
| const base = `https://${dmOrigin}/adobe/assets/${asset['repo:id']}`; |
There was a problem hiding this comment.
could we enable ${dmOrigin.includes('/') ? '' : '/adobe/assets/'} again in all places where /adobe/assets/ is appended/hardcoded? It enables that if aem.assets.prod.origin is used the base path can be overwritten:
aem.assets.prod.origin=example.org-> DM path ishttps://example.org/adobe/assets/urn:aaid:aem:...aem.assets.prod.origin=example.org/custom/path/-> DM path ishttps://example.org/custom/path/urn:aaid:aem:...
There was a problem hiding this comment.
I expect aem.assets.prod.origin to contain just the custom origin and not the base path. Even if it contains /, it doesn't guarantee that the base path is /adobe/assets/ IMHO. If we think that making the base path configurable adds value, I can create a new configuration for the base path.
@auniverseaway , thoughts?
There was a problem hiding this comment.
we support it today and I'll need it for my client, the idea was that as soon as an origin with path is configured it's up to the project to make sure the right base path is present
There was a problem hiding this comment.
If it is just one site, and I create a new configuration for the base path, you can then add the base path config for that site. Will that work? Because the configuration for aem.assets.prod.origin was meant to load the assets from a custom domain. Allowing the base path would not be ideal and may create confusion if adopted by more customers IMO.
There was a problem hiding this comment.
Added support for custom base path configuration aem.assets.prod.basepath. Defaults to '/adobe/assets' if not configured.
| if (mimetype.startsWith('image/')) { | ||
| return `${base}/as/${asset.name}`; | ||
| } | ||
| if (mimetype.startsWith('video/')) { |
There was a problem hiding this comment.
this will change how video URLs are handled currently, I think we need a config flag here:
- default can be
${base}/playto get the video player URL - if "disable video player" flag is set:
${base}/original/as/${asset.name}to enable direct URL with filename
There was a problem hiding this comment.
I like the idea of having a configuration to deliver the original file for videos. I can implement one.
@auniverseaway do you agree?
There was a problem hiding this comment.
maybe this can be combined with #832, looks like if that configuration can be customised we can handle also this case
There was a problem hiding this comment.
This has been implemented through aem.asset.mime.renditions configuration. Images will be rendered in .avif format, videos use /play and all other files will be loaded as original by default and can be overridden by configuring the aem.asset.mime.renditions as image/*:original,video/*:original
…da-live into asset-selector-refactor
…da-live into asset-selector-refactor
auniverseaway
left a comment
There was a problem hiding this comment.
This is really great stuff. Thanks for this.
Description
Problem
The existing
da-assets.jswas a single ~250-line function (openAssets) thatmixed config loading, URL construction, ProseMirror insertion, smart crop UI,
approval checking, and DOM management. This made it difficult to maintain,
extend, or test. It also had a correctness bug: DM Open API delivery assets use
repo:assetIdandrepo:repositoryIdin their response, but the old codeincorrectly read
repo:id(the author-tier field) for URL construction.Changes
Refactored
blocks/edit/da-assets/Split the monolith into four focused helper modules:
helpers/config.js— config fetch/cache,getConfKey,getRepositoryConfig,getResponsiveImageConfig.getRepositoryConfigis the new central functionthat resolves the three repository modes from the DA site config keys.
helpers/urls.js— three explicit URL builders, one per mode:buildAuthorUrl— author tier, publish URLs viaasset.pathbuildDmUrl— author tier + DM delivery, uses correctasset['repo:id']buildDeliveryUrl— delivery tier (DM Open API), usesrepo:assetId/repo:repositoryId/repo:nameper the AEM Open API spec/play), and otherformats (PDFs, CSVs →
/original/as/) to the right URL pattern.helpers/insert.js— ProseMirror helpers:insertImage,insertLink,insertFragment,createImageNode,findBlockContext,getBlockName.helpers/smart-crop.js— self-contained smart crop dialog with cleancallbacks (
onInsert,onBack,onCancel). Refactored crop list clickhandler from
li:hovertoe.target.closest('li')for reliability.da-assets.jsis now a thin ~50-line orchestrator. The NX/IMS dynamic importsare lazy (inside
openAssets) so the module is importable in unit tests.Bug fixes
&&to||: an asset nowrequires both
status === 'approved'andactivationTarget === 'delivery'to pass, not just one of them./playendpoint (same as delivery tier)instead of
/original/as/./original/as/path per the AEM Delivery API spec.getConfKeyis no longer re-exported fromda-assets.js;da-librarynowimports it directly from
helpers/config.js.Tests (
test/unit/blocks/edit/da-assets/)Added 5 new test files with 462 tests, bringing helper coverage to 100%:
helpers/config.jshelpers/urls.jshelpers/insert.jshelpers/smart-crop.jsda-assets.jsopenAssetsexcluded — IMS dependency)Configuration reference
All keys from https://docs.da.live/administrators/guides/setup-aem-assets
are supported:
aem.repositoryId,aem.assets.prod.origin,aem.assets.image.type,aem.asset.dm.delivery,aem.asset.smartcrop.select,aem.assets.renditions.select(reserved for future use).Related Issue
#842
Motivation and Context
How Has This Been Tested?
Tested using Chrome Dev Tools override option.
Screenshots (if appropriate):
Types of changes
Checklist: