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): invalidate queries if page context changes between runs #28351

Closed
wants to merge 1 commit into from

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Nov 29, 2020

This is successor to #28316 which had potential of causing OOM crashes when persisting/serializing data in case there was huge amount of data passed via page context.

Instead of persisting entire page objects this persists just hash generated from page context which should minimize how much more data will be persisted (and hopefully avoid OOM crashes)

TODO:

  • determine if
    const deletedPages = deleteUntouchedPages(store.getState().pages, timestamp)
    needs to be adjusted - right now it uses pages slice which is not persisted - so it's entirely possible that this will keep stale page context hashes around. Potential solution here would be to add timestamp to newly added reducer (maybe even just move current ones we have in pages to new one) and switch to using new (persisted) slice of redux state for finding pages that were not re-created.

Related Issues

fixes #28281
fixes #26520
[ch19791]

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Nov 29, 2020
@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 29, 2020
Copy link
Contributor

@vladar vladar left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍 At least I don't see any pitfalls with this approach.

@@ -806,7 +807,7 @@ describe(`query caching between builds`, () => {
}, 99999)
})

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

Choose a reason for hiding this comment

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

🎉

@pieh
Copy link
Contributor Author

pieh commented Dec 2, 2020

Ok, this is not sufficent change. As I noted in the description

determine if

const deletedPages = deleteUntouchedPages(store.getState().pages, timestamp)
needs to be adjusted - right now it uses pages slice which is not persisted - so it's entirely possible that this will keep stale page context hashes around. Potential solution here would be to add timestamp to newly added reducer (maybe even just move current ones we have in pages to new one) and switch to using new (persisted) slice of redux state for finding pages that were not re-created.

Just did run some manual tests for it. For cases when we:

  • run build 1 that creates "page A",
  • run build 2 without "page A",
  • run build 3 with "page A" re-added (with same context as in build 1)
    we don't rerun query in "build 3" and "build 2" deletes generated files - most importantly page-data ( to not show page that was "deleted")

This result in build error as follows:

failed Building static HTML for pages - 0.757s
 ERROR #95313 
Building static HTML failed for path "/context/b/"
See our docs page for more info on this error: https://gatsby.dev/debug-html
  Error: ENOENT: no such file or directory, open '/Users/misiek/dev/gatsby-starter-blog/public/page-data/context/b/page-data.json'
  - render-page.js:635 getPageData
    /Users/misiek/dev/gatsby-starter-blog/public/render-page.js:635:1022
  - render-page.js:640 Module../.cache/static-entry.js.__webpack_exports__.default
    /Users/misiek/dev/gatsby-starter-blog/public/render-page.js:640:1497
  - render-html.js:30 
    [gatsby-starter-blog]/[gatsby]/dist/utils/worker/render-html.js:30:36

So we do need to cover properly cleaning up reducer that we will persist

Copy link
Contributor

@vladar vladar left a comment

Choose a reason for hiding this comment

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

Just trying to discard my review after @pieh request

@pieh
Copy link
Contributor Author

pieh commented Dec 2, 2020

To add to my above comment - just adding timestamp and rest of fields that are used in findChangedPages / deleteUnotuchedPages doesn't seem sufficient, because after finding deleted pages we do emit DELETE_PAGE action which currently uses entire page object as payload. We can adjust internals to make sure things are fine. But various plugins might already subscribe to this event in some cases and while this technically wouldn't be breaking change, it could cause unforseen consequences for the ecosystem...

@pieh
Copy link
Contributor Author

pieh commented Dec 13, 2020

Ok, this is not feasible approach in the end due to above - I will actually use #28316 as a base and handle potential OOM issues by expanding persisting chunking to also chunk pages on top of nodes - #28590

Will be closing this PR

@pieh pieh closed this Dec 13, 2020
@LekoArts LekoArts deleted the fix/persist-page-context-hash branch April 23, 2021 11:07
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