-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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(gatsby-source-shopify): download images option #23840
feat(gatsby-source-shopify): download images option #23840
Conversation
Co-authored-by: Luke Bennett <hello@lukebennett.com.au>
Co-authored-by: Luke Bennett <hello@lukebennett.com.au>
Co-authored-by: Luke Bennett <hello@lukebennett.com.au>
(caching will make that defaultImage only download once) Co-authored-by: Luke Bennett <luke@lukebennett.com.au>
Co-authored-by: Luke Bennett <luke@lukebennett.com.au>
Any advice anyone what I can do to have this reviewed sooner? |
@mrhut10 Yes!! Been dreaming of this - same situation, many products (and the images are huge) so the whole app is failing right now and I can't boot it. I'd love to test this - I'm having a hard time adding your branch into my project - do you have any pointers there? edit: i ended up just plucking the shopify folder and pushing up to private repo as yarn dependency. this works perfectly - let's merge this bad boy i have like 4 projects i need this on! |
@beamercola reallly glad to hear you got it working and your keen for it. next project @lukebennett88 and I want to implement is a graphQL fragment which is compatible with gatsby-image that uses Shopify's CDN. would you also be keen on that in your projects @beamercola ?? |
@beamercola Glad you got it working! It's nice to see other people wanting this functionality as well, I think it could be very useful for a lot of people. |
hay @beamercola I've brought up to date before but not sure who's buttons we need to press to get it merged. to help our case to get merged, can you (and anyone else) describe the pain your having / problem this will solve. and on my end, I will relook at bring up to date next week and being it has been a while sence I've played with I think I better put it though its passes testing it on a demo project again. |
@mrhut10 My painpoint is I'm managing ~5 Shopify stores on Gatsby, some of which have multiple hundred products (each with ~5 images per product). Build time can be 20+ minutes. The annoyance is that I'm not using the local files/sharp for Gatsby, I'm relying on the CDN. The modifications you made were perfect, it saved my butt with some angry clients after I sold them on the promise of Gatsby. But a quick look at the I imagine there's a lot of Gatsby sites that include 5 products or so as a small "shop" section of the site. But the force downloading of all images is limiting to those who are excited about replacing Shopify with Gatsby for e-commerce. Thanks for your patch, it's still going strong but getting nervous as the codebase is diverging a bit and I'm unable to update to newer versions without open heart surgery |
@beamercola thank you so much for sharing your pain, sounds like your describing the exact circumstance that this branch / feature was made for when @lukebennett88 first thought it up. :) yay. but I haven't look much codebase with the changes they've recently done. I'll certainly try to have a look at it next week. |
Hi all. So sorry this hasn't gotten in yet. It's a good change. I tried merging master locally and it merged cleanly so it looks like this can get it easily still. @mrhut10 if you could give me write access to the PR, I can push the changes + do some testing and help get this in. |
Gladly and sounds exciting, @KyleAMathews |
there you go @KyleAMathews I've added you as a collaborator on my fork of the project :) |
Merged in master. We'll see how tests go. |
thank you @KyleAMathews I'll let you know once I make those changes :) |
just having a play with this now, also noticed in the future we might want to support turning off images for particular types of documents, i.e. Product Images, Collection Images, and Article Images if theres a needs for it could selectively turn this off. But I guess see if that gets requested after this base feature is in master. |
* removed and defaultUrl image config * removed the default.png image * fixed up documentation
* if !downloadImages then don't download image
Hay @KyleAMathews I've committed those changes from your review.
|
testing I've already done @KyleAMathews Setup Environment
Test Cases |
@beamercola / @GooBall / @lukebennett88 are any of you already using shopify CDN for images? once this is merged thinking it would be good to look at the possibility to:
|
Hey @mrhut10 I haven't looked into this for a while. To resize an image I would append e.g.: Original: Resized: I wrote a crude function to resize them which was good enough for how I was using it: function resizeShopifyImage({ url, width }) {
let fileExtension = '.jpg';
let index = url.indexOf('.jpg');
if (index === -1) {
index = url.indexOf('.png');
fileExtension = '.png';
}
if (index !== -1) {
const firstChunk = url.slice(0, index);
return firstChunk.concat(`_${width}x${fileExtension}`);
}
return url;
}
export { resizeShopifyImage }; I know at the time I there was an issue with the CDN — if I requested an image that was larger that the original image, the image wouldn't load. This doesn't seem to be the case anymore so a using this with Gatsby Image should probably work (I'm fairly certain that the base64 image isn't required since there is a Gatsby Image fragment with no base64 encoded image: One other thing to consider is that the Gatsby Image team are working on a replacement for Gatsby Image — Gatsby Plugin Image so it's probably a good idea to make it compatible with that so we don't have to redo it later. |
👍 to this. Here's an example of how source plugins can do the integration — #28236 |
@KyleAMathews is there anyone we can request from gatsbyjs/documentation to review? |
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.
Thanks! Sorry again for the slow merge.
I've got the power ;-) |
Yay!!!! so excited!! thank you @KyleAMathews @beamercola and/or @lukebennett88 can I use one of your sites that has too many shopify images in a blog post showing the build time performance increase? or/and interested in writing one yourself? (I still need to set up a blog or medium or something) |
@KyleAMathews for prez @mrhut10 yeah, happy to! |
So happy to see this merged! |
How do the npm releases work? |
@lukebennett88 its possible its automated, but otherwise I believe they would have to increment the version number in the package.json and then do a deployment to npm. in the meanwhile to run locally in your dev environment
If you want to run in production, you will need to do
please let me know if you want a hand to try / set up |
Thanks @mrhut10, yeah I know I could get it working manually, but didn't want to go through the extra work if the package is likely to be updated soon. Appreciate the write-up though, and thanks for going the last mile to finally get this thing merged in. So happy 🎉 |
@lukebennett88 looks like its been tagged and deployed under the tag 3.9.0-next.1 so you should be able to npm install that tag and use this feature with that version tag.
likewise you could always ask @beamercola to redeploy his npm version which he was using for his sites. |
Description
this PR aims to add a downloadImages option to the gatsby-source-plugin as described by @lukebennett88 in issue #23724
this prevents huge amounts of images being downloaded if you intended to use Shopify's CDN for images anyway greatly speeding up site build times.
This is my first PR to gatsby and neither @lukebennett88 or I feel it has had enough testing so hoping the community can test this implementation.
##Example Use
say you have a Shopify store with 500 products and 5 images per product. and you are happy to use Shopify's CDN (and transforming URLs) for the files. currently, the plugin will still download all of the images in this example that's 2500 images your downloading for no reason greatly slowing builds or timing out your build.
Documentation
new Options to gatsby-shopify-plugin
have changed the plugin readme file to reflect this @gatsbyjs/learning please read
Related Issues
should fix #23724
NOTE:
in the file "packages/gatsby-source/shopify/src/nodes.js" in the method call "downloadImageAndCreateFileNode"
do I need to do any cache invalidation in this file?
I have some concern that the referencing to cache with the fileNodeID may mean that when downloadImages is set back to true (from previously being false) it may try to use the cached file rather than the shopify images.