-
Notifications
You must be signed in to change notification settings - Fork 24
Enable support for custom defined CDN endpoints #97
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
Conversation
@@ -17,22 +17,29 @@ describe('cdn helper', function () { | |||
theme_version_id: '123', | |||
theme_config_id: '3245', | |||
}; | |||
var themeSettings = { | |||
cdn: { | |||
customcdn: { |
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 structure is unnecessary, the path is too nested. Can you change the code to be like this?
"settings": {
cdn: {
customcdn1: "cdn1.mystore.com",
customcdn2: "cdn2.mystore.com"
}
}
usage:
{{cdn "customcdn1:path/to/image.jpg"}}
{{cdn "customcdn2:path/to/image.jpg"}}
Also, if you can, add a test case with two custom cdn urls like the above
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 definitely agree that the additional nesting is unnecessary, but I was thinking it would be better to start out with an object instead of a string in the event additional functionality like different endpoints based on environment. For example, Imgix can resize images based on URL parameters, but if you wanted to test this functionality locally you'd need to run an additional server, and setup something like this
cdn: {
imgix: {
path: "https://bc.imgix.net",
local_path: "http://localhost:8080"
}
}
However, starting with using strings and then just adding a typecheck in the future if additional functionality is added could be a way around it as well! Just let me know what you'd prefer and I'll make it happen!
I'll also push a few additional tests with multiple endpoints configured.
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 understand your point, but we want to keep the settings very simple and clean. If we want to implement the different environments idea we could do the type check as you said.
also adds additional tests for when multiple CDN endpoints are configured
LGTM 👍 need to test it before deploying to production @bc-ranji |
Dude, sweet. |
Does it only work with cdn helper? How to do the same with getImage helper? |
|
Is there any possible way so that one can use custom CDN for the URLs generated via Maybe some sort of assignment and then replacing in the template. Not sure if that's possible in the handlebar. |
there is a |
This pull request allows theme creators to define custom CDN endpoints, and use them via the
cdn
handlebars helper.When designing themes for BigCommerce, theme designers might want to include large, high resolution image assets as components of their theme. For example on maruccisports.com, each product category includes a 5120x2536 background image, most of which are between 3 and 5 MB. Since sending that much image data down the wire is a poor user experience, we use Imgix to resize and reformat the images depending on the device viewing them. A theme designer might want to use a local version of the image in development, and a version on a CDN like Imgix in production. This pull request allows the designer to define endpoints in their theme's
config.json
file like this:After defining an endpoint, the theme designer is then able to use the shortname the same way they currently use the
webdav
one:In development, the previous helper would return the following:
And in production:
Right now, the helper is configured to rewrite local URLs to a
cdn
folder withinassets
. This is to circumvent the 50MB bundle size limit by excludingassets/cdn
from the bundle created bystencil bundle
. In addition to this PR, I've filed one that makes this change against the stencil-cli repo. bigcommerce/stencil-cli#240