refactor(gatsby): improve EnsureResources

[vtenfys]

• getDerivedStateFromProps
  • Remove unused parameter pageResources
  • Return even if we don't have resources, to allow detecting them later
• Remove componentDidUpdate and move some of its contents into new retryResources
• Throw an error on initial render only upon missing resources - otherwise just don't update the component, and remove the function shouldRenderStaticHTML
  • Note: this may need updating in the future - React docs say shouldComponentUpdate is only for performance purposes and may not prevent rendering in future versions
• Add a new function for detecting if we have resources which works for production and develop

[pieh]

I'm bit worried about updating state and then relying on shouldComponentUpdate to block the render if resources are not there yet - it seems fragile.

If I understand correctly this is to ensure we only reload during navigation if resources can't be loaded and not on initial render (to prevent infinite reload loop) - if so we can just store variable to see if we are doing initial render to avoid reload if it's initial render?

[pieh]

Also not really related to this PR, but something that I didn't caught in previous PR - there seems to be edge cases where:

const __html = document.getElementById(`___gatsby`).innerHTML
return <div dangerouslySetInnerHTML={{ __html }} />

doesn't work correctly - see https://5c0a7e2267610c06381781cc--vibrant-allen-8e8d7b.netlify.com/page-2/ (I just deleted .json for that page there to trigger this) - part of the page doesn't end up in the DOM - this would be caused by react hydration I would assume - when using this setInnerHTML we put more wrapping divs - one from @reach/router and one extra from <div dangerouslySetInnerHTML={{ __html }} /> itself. Unfortunately it doesn't seem we can use dangerouslySetInnerHTML without div or span ( facebook/react#12014 )

[vtenfys]

I'm bit worried about updating state and then relying on shouldComponentUpdate to block the render if resources are not there yet - it seems fragile.

If I understand correctly this is to ensure we only reload during navigation if resources can't be loaded and not on initial render (to prevent infinite reload loop) - if so we can just store variable to see if we are doing initial render to avoid reload if it's initial render?

Putting the check in shouldComponentUpdate is good because we can check if we have resources early on - but I agree it might be fragile with future versions of React, so I've added an additional check in the render function (so we don't have to rely on it blocking renders).

Also not really related to this PR, but something that I didn't caught in previous PR - there seems to be edge cases where: ... doesn't work correctly

I've updated it to throw an error rather than using dangerouslySetInnerHTML so we no longer have this problem - previously when I tested this, throwing an error broke navigation, but with this refactor links work okay.

[pieh]

Verified once again after updates. Nice one, thanks @davidbailey00!

[apvarun]

I was looking into the changes in this pull request for #10534 and it looks like throwing an error when the resource is not found results in 404 page not being displayed.

@davidbailey00 : Can you elaborate on the gain we get (prevent hydration) by throwing an error? Just trying to understand it.

[vtenfys]

@apvarun By throwing an error, we prevent React from rendering a blank page. Pages are server-rendered, so if we prevent React rendering then we will still see the page as rendered by the server, which is better than it being blank.

[apvarun]

I think we should not do this in dev environment as the errors get displayed on the screen for debugging. This prevents the gatsby's default page that lists all the existing routes from not displaying during development.

[vtenfys]

@apvarun This is now fixed in #10625 :)