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

Use StaleWhileRevalidate for page-data files #19342

Merged
merged 1 commit into from
Nov 12, 2019
Merged

Use StaleWhileRevalidate for page-data files #19342

merged 1 commit into from
Nov 12, 2019

Conversation

vtenfys
Copy link
Contributor

@vtenfys vtenfys commented Nov 7, 2019

Description

Use StaleWhileRevalidate for page-data for better performance and initial renders

Related Issues

Fixes #19207

@vtenfys vtenfys requested a review from a team as a code owner November 7, 2019 14:49
@KyleAMathews
Copy link
Contributor

Another option I was just researching is network first but with a timeout. Ideally we do get the latest data — we just don't want to wait that long for that because our top priority is consistently fast loading followed by strong consistency of data.

This issue has some sample code GoogleChrome/workbox#1738 (comment)

Perhaps a networkTimeout of 0.75 seconds? That would enable us to always render under a second.

@vtenfys
Copy link
Contributor Author

vtenfys commented Nov 8, 2019

Ideally we do get the latest data

Technically we should keep the page-data fetch logic the same as the logic for getting page bundles, since the data may change shape or physical appearance between two deploys of a site - so I think whichever we choose, we should use the same approach for JS/CSS as well. Thoughts?

@sidharthachatterjee
Copy link
Contributor

Technically we should keep the page-data fetch logic the same as the logic for getting page bundles

I agree with this for two reasons

  • A different page bundle might expect a different data shape and a mismatch could break the site during rehydration
  • Agree that this simplifies debugging

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.

Thanks, Dave!

@sidharthachatterjee sidharthachatterjee added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Nov 12, 2019
@gatsbybot gatsbybot merged commit e81ebb1 into gatsbyjs:master Nov 12, 2019
@KyleAMathews
Copy link
Contributor

Technically we should keep the page-data fetch logic the same as the logic for getting page bundles, since the data may change shape or physical appearance between two deploys of a site

Did we ever start refetching page (and app) bundles on a regular basis to check if they've been updated?

The big downside to this PR is that now page data won't be updated until the second time you visit a page as the browser will first get the cached page (however old it is) & then while the SW gets the newer version, the browser won't.

Also... on data changing shape. With the webpack compilation hash now not being in the page-data.json — how do we know what version of page data we're working with? Someone could have cached a version weeks ago and then the page component changed & we'd still try to use it which would throw an error...

This PR might need reverted.

@21c-HK
Copy link

21c-HK commented Jun 16, 2020

Quoting @KyleAMathews:

The big downside to this PR is that now page data won't be updated until the second time you visit a page as the browser will first get the cached page (however old it is) & then while the SW gets the newer version, the browser won't.

Also... on data changing shape. With the webpack compilation hash now not being in the page-data.json — how do we know what version of page data we're working with? Someone could have cached a version weeks ago and then the page component changed & we'd still try to use it which would throw an error...

This PR might need reverted.

That is exactly the problem with this PR. This PR must be reverted as it introduces extremely hard to reproduce caching bugs as explained here that ruin the user experience (by suggesting to the user that the web site is completely broken). This PR even contradicts the official documentation:

Similar to HTML files, the JSON files in the public/page-data/ directory should never be cached by the browser. In fact, it’s possible for these files to be updated even without doing a redeploy of your site. Because of this, browsers should be instructed to check on every request if the data in your application has changed.

This PR basically kills any kind of caching reliability for web sites in production, so this cannot be the right default. Gatsby feels so fast because of its aggressive caching and preloading, which is only employable if you can expect it to be reliable, but this PR makes Gatsby's caching unreliable.

@21c-HK
Copy link

21c-HK commented Jul 9, 2020

I clicked on a link to https://www.gatsbyjs.org/ today, which showed a blank (black) page. So there you have it. This caching bug even shows up on your own web site.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove blocking network request when loading site from a service worker (with gatsby-plugin-offline)
5 participants