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.

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

chore(gatsby-source-contentful): download assets via gatsby-core-utils #33482

Merged
merged 6 commits into from
Oct 13, 2021

Conversation

axe312ger
Copy link
Collaborator

This replaces all custom asset download code with gatsby-core-utils from Gatsby v4.

Primarily as companion for #33461.

We might only merge this for now and see how it behaves, but I am pretty sure we at least need reload on 429s and 503s in core which AFAICS is not in Gatsby core yet.

@axe312ger axe312ger requested a review from wardpeet October 10, 2021 10:00
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Oct 10, 2021
@wardpeet wardpeet force-pushed the feat/contentful-use-gatsby-core-utils branch from 8a47a37 to 9a542a9 Compare October 13, 2021 07:12
@axe312ger axe312ger force-pushed the feat/contentful-use-gatsby-core-utils branch from 9a542a9 to 59d93d6 Compare October 13, 2021 10:32
@wardpeet wardpeet force-pushed the feat/contentful-use-gatsby-core-utils branch from 59d93d6 to c7a2cfc Compare October 13, 2021 10:57
@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 Oct 13, 2021
@axe312ger axe312ger marked this pull request as ready for review October 13, 2021 11:13
@axe312ger
Copy link
Collaborator Author

The old code caches the base64 result files onto the file system. As we now use fetchRemoteFile, I am pretty sure we should be able to remove the following code and remove unnecessary I/O?

  • // TODO: Find the best place for this step. This is definitely not it.
    fs.mkdirSync(CACHE_IMG_FOLDER, { recursive: true })
    const cacheFile = path.join(CACHE_IMG_FOLDER, urlSha + `.base64`)
    if (fs.existsSync(cacheFile)) {
    // TODO: against dogma, confirm whether readFileSync is indeed slower
    const promise = fs.promises.readFile(cacheFile, `utf8`)
    inFlightBase64Cache.set(requestUrl, promise)
    return promise.then(body => {
    inFlightBase64Cache.delete(requestUrl)
    resolvedBase64Cache.set(requestUrl, body)
    return body
    })
    }
  • try {
    // TODO: against dogma, confirm whether writeFileSync is indeed slower
    await fs.promises.writeFile(cacheFile, body)
    return body
    } catch (e) {
    console.error(
    `Contentful:getBase64Image: failed to write ${body.length} bytes remotely fetched from \`${requestUrl}\` to: \`${cacheFile}\`\nError: ${e}`
    )
    throw e
    }

Also, am I correct that there are two additional memory caches?

  1. // Prefer to return data sync if we already have it
    const alreadyFetched = resolvedBase64Cache.get(requestUrl)
    if (alreadyFetched) {
    return alreadyFetched
    }
  2. // If already in flight for this url return the same promise as the first call
    const inFlight = inFlightBase64Cache.get(requestUrl)
    if (inFlight) {
    return inFlight
    }

I am sure these have been added by @pvdz for a good reason. Can we run/check the related benchmarks again?

wardpeet
wardpeet previously approved these changes Oct 13, 2021
@wardpeet wardpeet changed the title refactor: download assets via gatsby-core-utils refactor(gatsby-source-contentful): download assets via gatsby-core-utils Oct 13, 2021
@wardpeet wardpeet changed the title refactor(gatsby-source-contentful): download assets via gatsby-core-utils chore(gatsby-source-contentful): download assets via gatsby-core-utils Oct 13, 2021
@wardpeet wardpeet merged commit 3f2d581 into master Oct 13, 2021
@wardpeet wardpeet deleted the feat/contentful-use-gatsby-core-utils branch October 13, 2021 15:14
wardpeet added a commit to herecydev/gatsby that referenced this pull request Oct 29, 2021
axe312ger added a commit that referenced this pull request Nov 9, 2021
#33482)

Co-authored-by: Ward Peeters <ward@coding-tech.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: source-contentful Related to Gatsby's integration with Contentful
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants