fix(gatsby): reevaluate page query when page context is modified

[RomanHotsiy]

Description

When page context is updated by createPage the path is not marked as dirty and page query is not reevaluated resulting in the page not updating in develop mode.

It was already fixed by my in #11048 but it appeared again in v2.2.11 because of refactoring by @KyleAMathews in 91086d4

I added a basic test, but I'm not sure how to write a test to prevent this kind of regression in the future. If anyone has any ideas I would be happy to add a test.

Related Issues

Related to #6097, May be related to #11691

[RomanHotsiy]

Ping @pieh (you reviewed the previous PR)

[pieh]

Right. There is problem with this that I'm not sure if I noted in previous implementation.

This will work only when context is modified while gatsby develop is running. It won't catch cases of context changes between builds as we don't persist pages created by previous builds right now

[RomanHotsiy]

@pieh, I think that was fixed here:

gatsby/packages/gatsby/src/redux/actions/public.js

Lines 344 to 345 in 38eda62

	const contextModified =
	!!oldPage && !_.isEqual(oldPage.context, internalPage.context)

[pieh]

This is what I actually meant. oldPage will be there only when page is updated while gatsby develop is running. It won't catch context changes if you change your content that would cause context to change when gatsby develop is not running (because we don't persist pages data)

[RomanHotsiy]

@pieh thanks for clarification!

It won't catch cases of context changes between builds as we don't persist pages created by previous builds right now

Oh, I see. I just verified that this issue happens between builds:

• I run gatsby build
• I change somehow page context
• I run gatsby build and page is not updated since last time

That's a more severe issue... I don't know how to fix it as part of this PR though. Persisting page data is a separate bigger PR.

I believe we can't assume context to be changed every time when oldPage is not there as it may hurt performance. Am I right?

I believe this PR can be merged and a separate issue should be open for "between-builds" context changes. I will be able to work on this fix.

[RomanHotsiy]

Sorry to bother you @pieh... but this is a severe issue: build may produce wrong results without the user noticing it.

Could you at least let me know what is your plan on this? Won't you merge this PR without a complete fix? Should I start working on a complete fix?

Thanks in advance.

[pieh]

That's a more severe issue... I don't know how to fix it as part of this PR though. Persisting page data is a separate bigger PR.

I believe we can't assume context to be changed every time when oldPage is not there as it may hurt performance. Am I right?

I believe this PR can be merged and a separate issue should be open for "between-builds" context changes. I will be able to work on this fix.

Yeah, this is bigger issue. The fix might be tiny (i.e. just persist pages redux namespace) - but there is a lot of things to consider here, as right now Gatsby operates under assumption that when it bootstraps pages are empty.

Could you at least let me know what is your plan on this? Won't you merge this PR without a complete fix? Should I start working on a complete fix?

I think it makes sense to merge this in general, but I didn't have yet time to thoroughly review and see if it can potentially cause some regressions.

[RomanHotsiy]

After some using in production, I noticed a severe performance issue with my changes: instead of rerunning only specific page query it was rerunning all page queries of a specific component.
I now fixed that and force-pushed changes.

@pieh any chance to get it merged soon?

[sidharthachatterjee]

@RomanHotsiy This seems like a valuable fix. Tested and verified locally!

I've documented the issue for gatsby build in #16477

Let's go ahead and publish this for now! Thank you 🥇

[sidharthachatterjee]

Published in gatsby@2.13.54