Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: add cache to fetchRemoteFile and integrate with gatsby-source-contentful & gatsby-transformer-sqip #30391

Closed
wants to merge 33 commits into from
Closed

feat: add cache to fetchRemoteFile and integrate with gatsby-source-contentful & gatsby-transformer-sqip #30391

wants to merge 33 commits into from

Conversation

axe312ger
Copy link
Collaborator

@axe312ger axe312ger commented Mar 22, 2021 •
edited
Loading

Gatsby

  • feat: add inflight cache to createRemoteFile

Contentful

  • refactor: replaces custom asset download logic with createRemoteFile with some extra retry logic. Exposed for other plugins as fetchContentfulAsset
  • fix: GATSBY_CONTENTFUL_EXPERIMENTAL_REMOTE_CACHE custom fs cache will now be used for every interaction with Contentful Image API
  • fix: [gatsby-source-contentful] Could not getDominantColor from image TypeError: Unable to download asset from http://images.ctfassets.net/... Cannot read property 'verbose' of undefined fix moved to fix(contentful): asset download retry #33024

SQIP

  • refactor: use new fetchContentfulAsset function

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Mar 22, 2021
@axe312ger axe312ger added topic: source-contentful Related to Gatsby's integration with Contentful and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Mar 22, 2021
@axe312ger axe312ger force-pushed the refactor/contentful-asset-dowloading branch from 49dbf9e to 967e544 Compare April 29, 2021 10:07
@axe312ger axe312ger changed the title WIP - clean up Contentful plugin refactor(contentful): replace custom image caching with fetchRemoteFile Apr 29, 2021
@axe312ger axe312ger marked this pull request as ready for review April 29, 2021 10:12
@axe312ger axe312ger added the status: needs core review Currently awaiting review from Core team member label Apr 29, 2021
@axe312ger axe312ger force-pushed the refactor/contentful-asset-dowloading branch from 967e544 to e4eda3d Compare May 12, 2021 08:17
@axe312ger axe312ger force-pushed the refactor/contentful-asset-dowloading branch from e4eda3d to 78ed287 Compare June 23, 2021 08:48
@axe312ger axe312ger requested a review from wardpeet June 29, 2021 08:13
@axe312ger
Copy link
Collaborator Author

Hmm somehow it now wants to connect to localhost. Will check whats going wrong here. I restart the CI now in case that was an CI (network) error.

@wardpeet
Copy link
Contributor

wardpeet commented Jul 1, 2021

@axe312ger is the image URL referring to https://localhost? or maybe you mocked fetchRemoteFile?

@axe312ger axe312ger force-pushed the refactor/contentful-asset-dowloading branch from 5f941d6 to fd1b66e Compare July 6, 2021 15:16
@axe312ger axe312ger mentioned this pull request Jul 13, 2021
Closed
wardpeet
wardpeet reviewed Jul 15, 2021
View reviewed changes
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small comments, almost good to go. Is this being tested in the e2e test?

packages/gatsby-core-utils/src/fetch-remote-file.ts Outdated Show resolved Hide resolved
@axe312ger
Copy link
Collaborator Author

@wardpeet should we more the cache with the original fetchRemoteFile function? @KyleAMathews voted on merging in a conversation we had

@axe312ger
Copy link
Collaborator Author

@wardpeet yes, it is covered by e2e

@axe312ger
Copy link
Collaborator Author

axe312ger commented Jul 19, 2021 •
edited
Loading

@wardpeet while rewriting I figured out that base64 generation was not covered by the PR.

I rewrote those parts, now everything is using fetchRemoteFile. Additionally I had to find out that fetchRemoteFile does not retry on server errors.

As we should only request 100 assets at once from the Contentful Image API, I rewrote my old wrapper to use fetchRemoteFile, limit to 100 requests at the same time plus retry logic for server errors. I could now remove the axios dependency as well.

In short: download-with-retry.js got a rewrite to use fetchRemoteFile and is now called fetch-contentful-asset.js

@axe312ger axe312ger changed the title refactor(contentful): replace custom image caching with fetchRemoteFile feat: add cache to fetchRemoteFile and integrate with gatsby-source-contentful & gatsby-transformer-sqip Jul 21, 2021
@axe312ger
Copy link
Collaborator Author

Do we have issues with the window temp dir? https://app.circleci.com/pipelines/github/gatsbyjs/gatsby/67858/workflows/a4b66993-8470-49a9-b442-d061d954cabc/jobs/766873?invite=true#step-107-1231

@wardpeet wardpeet force-pushed the refactor/contentful-asset-dowloading branch 2 times, most recently from e88c89b to 7b12c96 Compare August 20, 2021 14:48
@wardpeet wardpeet force-pushed the refactor/contentful-asset-dowloading branch 2 times, most recently from 2ae4698 to 3f686a0 Compare September 1, 2021 10:11
@axe312ger axe312ger force-pushed the refactor/contentful-asset-dowloading branch from f441cb3 to 8bda979 Compare September 2, 2021 13:18
@wardpeet wardpeet mentioned this pull request Sep 13, 2021
Merged
@axe312ger axe312ger marked this pull request as draft September 17, 2021 14:45
@axe312ger
Copy link
Collaborator Author

@wardpeet thank you for the releases, it works fine on my projects as the number of CI fails got reduced.

I'll set this to draft and clear up this PR from noise.

@axe312ger axe312ger mentioned this pull request Sep 17, 2021
Merged
@axe312ger axe312ger mentioned this pull request Oct 7, 2021
Merged
3 tasks
@axe312ger
Copy link
Collaborator Author

closed in favour of #33461

@axe312ger axe312ger closed this Oct 7, 2021
@axe312ger axe312ger deleted the refactor/contentful-asset-dowloading branch November 5, 2021 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wardpeet wardpeet wardpeet left review comments

Assignees
No one assigned
Labels
status: needs core review Currently awaiting review from Core team member topic: source-contentful Related to Gatsby's integration with Contentful
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@axe312ger @wardpeet