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): fix infinite reloads when resources fail #10141

Merged
merged 5 commits into from
Nov 28, 2018
Merged

fix(gatsby): fix infinite reloads when resources fail #10141

merged 5 commits into from
Nov 28, 2018

Conversation

vtenfys
Copy link
Contributor

@vtenfys vtenfys commented Nov 26, 2018

  • Check if resources fail (as before)
  • Upon any failure, render static HTML instead of dynamically rendering the children
    • Upon initial failure, reload the page and mark the path as failed
    • Upon second failure, don't reload the page and unflag it (since it might have just been a temporary failure and we want it to retry the next time)

Fixes #10074

@vtenfys vtenfys requested a review from a team as a code owner November 26, 2018 16:45
@pieh
Copy link
Contributor

pieh commented Nov 27, 2018

I'm not sure about using innerHTML - it will work for initial mount, but afterwards it would prevent page transitions if resources for next page failed to load? (as html markup hold in state is from first page). I'm thinking if we shouldn't just fail there (i.e. throw error)

@vtenfys
Copy link
Contributor Author

vtenfys commented Nov 27, 2018

afterwards it would prevent page transitions if resources for next page failed to load? (as html markup hold in state is from first page)

If we're online this won't happen, because upon the first failure we immediately reload the page - upon second failure (i.e. after reloading) we've already loaded the full HTML for the page which failed, so rendering static HTML will work correctly.

However, if we go offline then later come back online and try to visit that page, this could happen (since the flag will still be set) - so I agree with you it might be better to just throw an error. I'll do some more testing later and try out these different scenarios.

@vtenfys
Copy link
Contributor Author

vtenfys commented Nov 28, 2018

@pieh I did some testing and I think the current approach works best - if we throw an error (instead of returning static HTML), links stop working, because we've thrown an error internally, whereas returning static content doesn't cause this problem.

Also, re: the "offline then revisit" scenario which I thought might be a problem - it actually works fine, because we remove the flag on any subsequent page load (not necessarily just the same page).

@pieh
Copy link
Contributor

pieh commented Nov 28, 2018

I'll pull this and do some local testing and hopefully merge soon

@pieh
Copy link
Contributor

pieh commented Nov 28, 2018

kapture 2018-11-28 at 12 23 14
This here show slight problem - when I start on /docs - html from that page is stored (constructor for EnsureResources is called just once - on first page). My second navigation (going to gatsby-starter-blog detail page) is when there are problems with loading resources and we get flash on very first page before we reload page.

Maybe getting innerHTML should be done straight in render function (yikes), so we don't flash stale content there? This is edge case, so I'd rather avoid updating state on each page transition here

Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

🙏 Thanks @davidbailey00!

@pieh pieh merged commit 9fc25f3 into gatsbyjs:master Nov 28, 2018
pieh added a commit that referenced this pull request Nov 28, 2018
gpetrioli pushed a commit to gpetrioli/gatsby that referenced this pull request Jan 22, 2019
- Check if resources fail (as before)
- Upon any failure, render static HTML instead of dynamically rendering the children
  - Upon initial failure, reload the page and mark the path as failed
  - Upon second failure, don't reload the page and unflag it (since it might have just been a temporary failure and we want it to retry the next time)

Fixes gatsbyjs#10074
gpetrioli pushed a commit to gpetrioli/gatsby that referenced this pull request Jan 22, 2019
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