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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(gatsby): add pages to saved redux state #28316

Closed
wants to merge 2 commits into from
Closed

fix(gatsby): add pages to saved redux state #28316

wants to merge 2 commits into from

Conversation

redabacha
Copy link
Contributor

@redabacha redabacha commented Nov 27, 2020

Description

it looks like pages is needed for gatsby to figure out if the page context has been modified since the last build:

const oldPage: Page = store.getState().pages.get(internalPage.path)
const contextModified =
!!oldPage && !_.isEqual(oldPage.context, internalPage.context)

but pages isn't included during saveState 馃

Related Issues

fixes #28281
fixes #26520

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Nov 27, 2020
@pieh pieh self-assigned this Nov 27, 2020
@pieh
Copy link
Contributor

pieh commented Nov 27, 2020

Hey @redabacha!

This is implementation of the fix is pretty unsafe to do, due to some sites putting enourmous amount of data via page context. This can cause cache persistance to not work at all in those cases (and actually cause fatal, unrecoverable crashes). We had issues like that before when persisting nodes which forced us to chunk what we save ( #21555 ), but that chunking wouldn't be covered for pages.

We do have idea how to solve this by persisting not entire page objects, but rather just some "hash" generated from page context. So instead of comparing contexts between old page and new page, we would compare newly generated hash with hash stored for previous page. This will require adding seperate reducer (something like pageContextHashes), that would react to CREATE_PAGE and DELETE_PAGE actions (to keep it in sync with actual pages that are alive).

createPage action should generate this hash and provide it when emitting CREATE_PAGE action as part of payload that will be stored in this new reducer.

Then it would be only matter of adding the new reducer state to list of things we persist (so similar as you did in your PR, just not entire pages slice of state)

@pieh pieh added topic: query invalidation* and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Nov 27, 2020
@pieh
Copy link
Contributor

pieh commented Nov 27, 2020

Oh - and lastly - this should also allow us to "unskip" this test scenario -

describe.skip(`Changing page context invalidates page queries`, () => {

@redabacha
Copy link
Contributor Author

redabacha commented Nov 27, 2020

hi @pieh

thanks for the explanation, it makes much more sense now. we also store most of our data via page context when building the site (https://www.hollandandbarrett.com/) so i'm glad this issue is being thought about. as it stands currently, upgrading gatsby (to 2.26+ i think) is not feasible due to this bug as the pages do not update when using cache from a previous build.

@pieh
Copy link
Contributor

pieh commented Nov 29, 2020

Hey @redabacha - I opened draft PR using idea I menioned in previous comments - #28351 . I did found some extra considerations that we will need to make (as noted in PR), but it actually did allow me to unskip the test scenario ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants