-
Notifications
You must be signed in to change notification settings - Fork 40
fix(cdn): MERC-9364 Use CDN Original images for webdav - cache control #298
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: master
Are you sure you want to change the base?
Conversation
1d04321
to
8a330f5
Compare
06cc938
to
a52423f
Compare
a52423f
to
46c6920
Compare
♻️ I have tested this with |
@bigcommerce/team-merchandising @bigcommerce/team-storefront |
82294e2
to
3344da0
Compare
8b8ca19
to
c9f8d6b
Compare
de37add
to
8b2e272
Compare
8b2e272
to
0483045
Compare
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.
LGTM
0483045
to
5944c24
Compare
5944c24
to
e1923c0
Compare
f766ffe
to
1e15c95
Compare
1e15c95
to
397d05d
Compare
397d05d
to
5fd2f7c
Compare
return [cdnUrl, 'content', path].join('/'); | ||
const supportedImageExtensions = ['jpg', 'jpeg', 'gif', 'png']; | ||
const unsupportedPathSyntaxes = ['../', './']; | ||
const searchParamIgnoredPath = path.split('?')[0]; |
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.
we should cover cases when pathname contains searchParams like
'webdav:/img/brands-logos/main.png?cache_bust=1749005114992'
and exclude them from filtering on the next step
const supportedImageExtensions = ['jpg', 'jpeg', 'gif', 'png']; | ||
const unsupportedPathSyntaxes = ['../', './']; | ||
const searchParamIgnoredPath = path.split('?')[0]; | ||
const isImage = supportedImageExtensions.some(ext => searchParamIgnoredPath.toLowerCase().endsWith(ext)); |
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.
previously used method includes()
can lead to incorrect results. For example, if provided path looks like 'webdav:banners/menu-giftcard.svg'
or 'webdav:/collection/gifts/image.pdf?cache-buster=2025-06-04T00:50:40'
then we will get situation where path.toLowerCase().includes(ext))
return true
and helper modified path for unsupported content.
Now the logic has been updated, so firstly we exclude searchParams from path and then check if it ends with supported extension.
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.
nice, looks good 👍
@@ -56,7 +56,14 @@ module.exports = globals => { | |||
} | |||
|
|||
if (protocol === 'webdav:') { | |||
return [cdnUrl, 'content', path].join('/'); | |||
const supportedImageExtensions = ['jpg', 'jpeg', 'gif', 'png']; |
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.
Should we include .
?
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 no, Jay. On L61 I exclude any searchParams from path and what is left I check if it ends with supported extension. So for example User provides something like: webdav:/collection/gifts/image.pdf?cache-buster=2025-06-04T00:50:40
.
We then check first if it webdav, then go and look for any searchParams and drop them so receive path: /collection/gifts/image.pdf
and then check if it ends with supported extensions.
jfyi, I would like to reiterate and summarise changes related to webdav functionality:
|
5fd2f7c
to
ac747ec
Compare
ac747ec
to
0dd26e2
Compare
What? Why?
! DO NOT MERGE !
Use CDN Original images for webdav - cache control. Take 2. See bigcommerce/paper#343
If a developer/merchant uses the cdn handlebars helper to inject images from webdav into the theme using e.g.
({{cdn "webdav:/img/image.jpg"}})
, there is no cache-control header when the image is fetched on the storefront.Similar to our Image Manager, we should be using the cdn original images (Similar to Jinsoo's PR here MERC-7127)
This PR differs from the previous as it only does this for file suffixes that image manager supports (jpg/jpeg/png/gif). This avoids the issue where the url is changed for e.g. a pdf, and thus creates a broken link.
How was it tested?
Tested locally and in integration, and with unit tests. Have verified that images (png/gif/jpeg/jpg) uploaded to the content directory via webdav can be accessed at their cache-controlled url using the
cdn
helper (/images/stencil/original/content/<whatever>
), while all other file extensions still return the/content/<whatever>
path when using the helper.Behold, the results of outputting some of these urls directly to the DOM:

cc @bigcommerce/storefront-team