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

feat(gatsby): Support all caching headers for createRemoteFileNode #31408

Closed

Conversation

cameronbraid
Copy link
Contributor

@cameronbraid cameronbraid commented May 13, 2021

When using createRemoteFileNode the origin server is always pinged, even for requests with a cache-control: public, max-age=5000 header.

This can add significant amount of time to a site build when hundreds to thousands of files are requested if each takes hundreds of ms (for a 304 response)

This PR proposes to use got's built in cache handling to avoid such cases.

Would gatsby consider something like this ?

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label May 13, 2021
@KyleAMathews KyleAMathews changed the title implement more aggressive caching option for createRemoteFileNode feat(gatsby): Support all caching headers for createRemoteFileNode May 13, 2021
@KyleAMathews
Copy link
Contributor

haha yes actually! I'd just noticed a week or so ago that we were only use etag and that this didn't always work (I happened to be testing grabbing a file from a server that didn't set it) and then realized the same thing, that we'd be able to avoid all the revalidations — #28331 (reply in thread)

I did a bit of research and was thinking that we'd want to implement our own lower-level cache to avoid reading/writing the file each time but perhaps got's caching is smart enough to avoid that (or maybe your implementation avoids this) — I didn't research much.

@LekoArts LekoArts added topic: source-plugins Relates to the Gatsby source plugins (e.g. -filesystem) and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels May 17, 2021
@cameronbraid
Copy link
Contributor Author

Looks like got's cache stored the response body as base64, and it doesnt look like theres an easy way to workaround that.

It may be best to look at integrating https://www.npmjs.com/package/http-cache-semantics instead

@KyleAMathews
Copy link
Contributor

Hmm yeah — that's really inefficient for our use case.

@cameronbraid
Copy link
Contributor Author

Another option could be to switch to using https://www.npmjs.com/package/make-fetch-happen (just for remote file download) as it's cache manager is based on https://developer.mozilla.org/en-US/docs/Web/API/Cache which gives direct access to the request and response objects meaning so it should be fairly straightforward to implement a cache manager that stores headers in gatsby cache and body in files

@cameronbraid
Copy link
Contributor Author

After some more research it looks like the got based cache does support custom Serializer/Deserializer so I will look into that approach first.

@KyleAMathews
Copy link
Contributor

Been mucking around with Got today with #32012 — basically it doesn't give you good low-level control over headers or the body. I don't see a way for us to make this PR work with Got so make-fetch-happen might be the best way forward.

@LekoArts
Copy link
Contributor

Hi @cameronbraid 👋

I'm closing this PR as stale as we've moved a lot of logic around in https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-core-utils/src/fetch-remote-file.ts and as it stands the current state of the PR won't be merged. It this is still something you need, please open a feature request first: https://github.com/gatsbyjs/gatsby/discussions/categories/ideas-feature-requests

Thanks!

@LekoArts LekoArts closed this Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: source-plugins Relates to the Gatsby source plugins (e.g. -filesystem)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants