-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
feat: return absolute media URLs from GraphQL #4565
feat: return absolute media URLs from GraphQL #4565
Conversation
const { metadata, large, medium, image, small, thumbnail } = mediaItem; | ||
|
||
const { priority, toGrid, productId, variantId, URLs: { large, medium, original, small, thumbnail } } = mediaItem; | ||
const absoluteUrl = Reaction.absoluteUrl().slice(0, -1); |
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.
@aldeed Is this all that's needed? How can I take into account when a CDN is being used?
…soluteUrl() added to context
…reaction into feat-4543-dancastellon-absolute-media-urls
primaryImage: (node, args, context) => xformProductMedia(node.primaryImage, context), | ||
variants: (node, args, context) => node.variants.map((variant) => { | ||
variant.media = variant.media.map((mediaItem) => xformProductMedia(mediaItem, context)); | ||
variant.primaryImage = xformProductMedia(variant.primaryImage, context); |
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.
Variants can have options, which have the same schema as variants. if (variant.options)
then do the same map on the options.
* @param {String} request.protocol Either http or https | ||
* @returns {String} URL | ||
*/ | ||
export default function getAbsoluteUrl(request) { |
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.
Similar to how the current Reaction.absoluteUrl
function works, this should take an optional path
arg and then do the work of properly appending that path. It should check if it starts with /
and remove it so that we don't end up with two /
.
In your xform you will then do something like large: context.getAbsoluteUrl(large)
.
Actually, performance wise, it's best if you have two different functions. This function you have now would be called only one time in buildContext
. You could actually set the result of that on context, too, as rootUrl
. Then context.getAbsoluteUrl
would only do the work to combine root url with path. Something like:
function buildContext() {
context.rootUrl = getRootUrl(request); // this is the function you currently have, renamed
context.getAbsoluteUrl = (path) => getAbsoluteUrl(context.rootUrl, path); // this just combines the strings intelligently
}
@aldeed Thanks, changes made! |
Not merging yet because we should merge this around the same time as the starter kit PR: reactioncommerce/example-storefront#243 |
Resolves #4543
Impact: minor
Type: feature
Issue
Currently GraphQL returns relative media URLs. Because of this, storefront needs to prepend the returned paths with the Meteor server's base URL.
Solution
Implement the currently unused
xformProductMedia
function in resolvers to prepend media URLs with the server's base URL. This will allow us in the future to take account for CDNs.Breaking changes
None
Testing