fix: MERC-9364 Use CDN Original images for webdav - cache control #298
Conversation
christensenep
force-pushed
the
MERC-9364
branch
from
1d04321
to
8a330f5
Compare
christensenep
changed the title
christensenep
force-pushed
the
MERC-9364
branch
2 times, most recently
from
06cc938
to
a52423f
Compare
helpers/lib/cdnify.js
Outdated
| const imgRegex = /(jpg|jpeg|gif|png)$/i; | ||
| const isImage = imgRegex.test(path); | ||
| const prefix = isImage ? 'images/stencil/original/content' : 'content' | ||
| return [cdnUrl, prefix, path].join('/'); |
There was a problem hiding this comment.
I feel like a logic as below is safer as it doesn't assume the image extension. What is your thought?
if (path.length >= colonIndex + 5 && path[colonIndex+5] == '/') {
const assetType = path.slice(colonIndex + 1, colonIndex + 5);
let prefix = 'content'
if (assetType == 'img/') {
prefix = 'images/stencil/original/content';
}
return [cdnUrl, prefix, path].join('/');
} else {
return [cdnUrl, 'content', path].join('/');
}There was a problem hiding this comment.
I'm not aware that imagers need to be in the img path like that, if they are then I would agree. But just looking at my stores image manager images, they are not.
There was a problem hiding this comment.
If we can indeed rely on webdav images always being in that path then yeah that's definitely way better. (And perhaps most importantly, rely on ONLY images being there)
There was a problem hiding this comment.
There might be more types we might be missing. But we could make sure we cover all the supported in this https://developer.mozilla.org/en-US/docs/Web/Media/Formats/Image_types
There was a problem hiding this comment.
I was erring on the side of being conservative just given the history here, but yeah I may be overcompensating. Curious as to the opinion of storefront.
Also realized I need to regex to match the . otherwise it'll match a file just named myjpg
christensenep
force-pushed
the
MERC-9364
branch
from
a52423f
to
46c6920
Compare
|
♻️ I have tested this with |
|
@bigcommerce/team-merchandising @bigcommerce/team-storefront |
| @@ -56,7 +56,10 @@ module.exports = globals => { | |||
| } | |||
|
|
|||
| if (protocol === 'webdav:') { | |||
| return [cdnUrl, 'content', path].join('/'); | |||
| const imgRegex = /.(jpg|jpeg|gif|png)$/i; | |||
There was a problem hiding this comment.
do we have somewhere documented the list of supported extensions?
There was a problem hiding this comment.
Public docs are here: https://support.bigcommerce.com/s/article/Using-the-Image-Manager?language=en_US#requirements (referring to image manager, but it's the same list as webdav-uploaded images). The actual list in bcapp is https://github.com/bigcommerce/bigcommerce/blob/d1ea38e376fd8237a8c721b801ea7e6c36c3f87a/lib/Store/Controllers/Stencil/BaseImageController.php#L32
| @@ -194,19 +194,40 @@ describe('cdn helper', function () { | |||
| ], done); | |||
| }); | |||
|
|
|||
| it('should return a webDav asset if webdav protocol specified', function (done) { | |||
| it('should return an original cdn img asset if webdav protocol specified but file type indicates it is an image', function (done) { | |||
There was a problem hiding this comment.
Just wondering if we can create functional tests for such cases?
What? Why?
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
cdnhelper (/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