fix(gatsby): Improve error message when EnsureResources fails to ensure a resource

[blainekasten]

Description

Documentation

It turns out that in rare cases, EnsureResources can fail. It's not currently known what can cause it. But the outcome of when it fails is that we hit an error accessing properties on an undefined object. https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby/cache-dir/root.js#L75

So this fix at least throws an error in render and doesn't try to render the app causing misleading errors.

Related Issues

Addresses #19959

[blainekasten]

this flag is so we can do react stuff. it was throwing an error and breaking builds

[pieh]

I have reservation about this - this will hide actual error in the runtime so it's more difficult to try to debug things. This is particularly problematic when the service worker is involved (because state of service worker caches etc is not sharable, and actual errors is only thing we can use to try to get to the bottom of things - it's still not easy, and sometimes even impossible to do, but at least there might be some clues).

So I don't know if hiding error here is good idea?

[blainekasten]

@pieh I may be wrong. But I don't think this is really making any functional difference from what is happening currently besides that the error it reports is more helpful.

Currently without our change, we call the render this.props.children(this.state). And if this.state has a blank pageResources, then we just crash here instead: https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby/cache-dir/root.js#L75

Since this code is assuming pageResources absolutely exists it crashes. And then the error is more obtuse. This change at least lets the user have a bit more context. We could add a detail to the log that says to wipe the .cache dir and rebuild to fix it

[pieh]

Let's not duplicate this line in warning and error message - I would vote to get rid of warn completely and just put this whole text in thrown error - sounds reasonable?

[pieh]

Ah I was mostly thinking about production builds here - maybe we should have conditional logic for gatsby develop (using process.env.NODE_ENV === `production` to show short(ish) message, but for gatsby develop case we can show cleaning up .cache (but it probably should be both .cache and public given stale page-data being on of potential reasons for this)

[pieh]

Hmmm, ok - I can agree with that.

I'm not sure if we should mention wiping .cache - this is browser error that end users of site can see and not just site developers (tho react will show stuff like "Error: Minified React error #130" (my favourite react error ;P) so maybe this is fine thing to do.

Tho with recent discovery that error like this is caused by stale page-data.json file - we should be able to just fix this (and maybe remove that error - it's always few bytes that might not need to be there :P)

[wardpeet]

Added small comment, after that I think it's good to go

[wardpeet]

Fixed in gatsby@2.19.24