-
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-contentful): Support storing assets locally #10682
feat(gatsby-source-contentful): Support storing assets locally #10682
Conversation
Downloads and caches Contentful Assets to the local filesystem. Useful for reduced data usage or projects where you want the assets copied locally with builds for deploying without links to Contentful CDN.
Filename was updated to match naming convention, missed updating it's import statement in `gatsby-node.js`.
912c3f3
to
4a87047
Compare
4a87047
to
c24051d
Compare
I'm not sure what's causing that or how to fix it. Navigating my own project works fine. Any advice? |
@polarathene don't worry about that e2e failure--it's unrelated. I'll create an issue now so that we can track on it. |
Whoops--I already did create that issue. See #9233, but once more, unrelated to any changes here! |
@DSchau @sidharthachatterjee Would either of you be happy to review and merge this then if the failing e2e test is nothing to worry about? :) I'm not sure if it will work well if an asset is modified on Contentful, it might not invalidate the cache and either not update the asset locally or create a new file copying over old data while the cache hasn't been cleared? (I have mentioned this in the README.md) The wordpress plugin uses a modified metadata value to handle this, Contentful isn't providing one for ContentfulAsset afaik, there is |
@polarathene just to make sure we won't have any regressions in the future could you add a few tests to test your cache invalidation code (https://github.com/gatsbyjs/gatsby/pull/10682/files#diff-caa2171e85ea277bf3075bd98f17c16eR51)? |
@wardpeet I could try, but I've not really written much tests before. It's been a while since I wrote this, I vaguely remember referencing some existing implementations(although I'm not sure that they had any testing in place either). Any advice(link to a similar test could suffice). |
It's more or less testing your download function to make sure it's only called when options.localDownload is set and if the correct nodes are created correctly. I have only found one package that tests sourceNodes https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-source-drupal/src/__tests__/index.js so I understand that this won't help you much as contentful doesn't have a lot of tests either. I don't want to burden you with this so if you're not up for it, no biggy. |
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! I'm going to test this one and I'll be merging afterwards
Tested this on www.gatsbyjs.com and seems to work. Looks identical to https://www.gatsbyjs.org/docs/image-tutorial/ 👍 |
Holy buckets, @polarathene — we just merged your PR to Gatsby! 💪💜 Gatsby is built by awesome people like you. Let us say “thanks” in two ways:
If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’. Thanks again! |
Downloads and caches Contentful Assets to the local filesystem. Useful for reduced data usage or projects where you want the assets copied locally with builds for deploying without links to Contentful CDN.
Resolves #10587 #5919