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

fix(gatsby-source-filesystem): Do not re-download cached files from createRemoteFileNode #12054

Conversation

skinandbones
Copy link
Contributor

Description

Fixes multiple issues in createRemoteFileNode that is causing the cache mechanism to not work correctly:

  1. Pass the headers option in got() correctly.
  2. Only encache response headers on success responses (so we don't overwrite the Etags – these are not returned in the 304 responses).

Related Issues

Fixes #12053

There are some issues reported in gatsby-source-wordpress about large downloads on restarts/builds that sound like they could be related.

Copy link
Contributor

@sidharthachatterjee sidharthachatterjee left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you, @skinandbones 👍

@@ -145,7 +145,7 @@ async function pushToQueue(task, cb) {
const requestRemoteNode = (url, headers, tmpFilename) =>
new Promise((resolve, reject) => {
const responseStream = got.stream(url, {
...headers,
headers,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch!

// Save the response headers for future requests.
await cache.set(cacheId(url), response.headers)

if (response.statusCode == 200) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to handle any other successful response status codes? Like 201 etc

I noticed that we also check for only 200 later in the file when moving temp data over so I'd guess we should be okay.

@sidharthachatterjee
Copy link
Contributor

Merging this in because I think it's good for now with the check for 200. We'll revisit this in case we find that it's not.

Still a great fix of course! Thank you @skinandbones 👍

@sidharthachatterjee sidharthachatterjee merged commit a358239 into gatsbyjs:master Feb 25, 2019
@gatsbot
Copy link

gatsbot bot commented Feb 25, 2019

Holy buckets, @skinandbones — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. (Currently we’ve got a couple t-shirts available, plus some socks that are really razzing our berries right now.)
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

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!

mwfrost pushed a commit to mwfrost/gatsby that referenced this pull request Apr 20, 2023
…reateRemoteFileNode (gatsbyjs#12054)

* Correct headers option for got() in createRemoteFileNode.

* Only encache response headers for success responses.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants