fix: check for dirty pages when nodes are deleted (so queries are ru-run and data is removed from pages) #11831

KyleAMathews · 2019-02-17T06:50:19Z

Bug reported @ https://twitter.com/gregwhitworth/status/1096882065943883777?s=19

…run and data is removed from pages)

KyleAMathews · 2019-02-17T06:52:41Z

Ha wait. @pieh reverted this exact code last August #7388 🤔

KyleAMathews · 2019-02-17T06:57:23Z

API_RUNNER_QUEUE_EMPTY is definitely not being called when deleting the markdown file.

KyleAMathews · 2019-02-17T07:00:11Z

The problem with #7388 was it assumed that all DELETE_NODEs would lead to API calls — which in simple node deletions it doesn't.

How to detect though when just a delete happens and nothing else.

KyleAMathews · 2019-02-17T07:10:34Z

So funny solution but perhaps a good idea is we could add a onDeleteNode API hook — which would mean API_RUNNING_QUEUE_EMPTY would now always be called 😎

KyleAMathews · 2019-02-17T07:20:07Z

I think I work around #7300 by checking that the DELETE_NODE node type is File as files are only deleted by gatsby-source-filesystem when a file is actually deleted. This wouldn't lead to any sort of infinite loop as if the file was immediately recreated, then runCreatePages wouldn't get called again until the new nodes are finished being created.

The problem with this is that then we'd need to add support for each new source plugin that deletes nodes. So onDeleteNode seems like a strong solution.

pieh · 2019-02-17T15:14:26Z

This is problem with any stateful source plugin - in our monorepo only filesystem behaves like that - but sanity plugin (with live data refresh feature enabled) that doesn't rely on sourceNodes API hook for deleting nodes has exactly same problem.

This only happens for node deletion, because node updates trigger onCreateNode API to run which then will emit API_RUNNER_QUEUE_EMPTY (this doesn't happen for node deletion).

We were discussing and pairing on this with @sidharthachatterjee earlier and my idea was to "fake" API running after DELETE_NODE so we could use existing API_RUNNER_QUEUE_EMPTY event handling. This would make sure we don't trigger creating pages / query running if deleteNode was called during API run (i.e. in onCreatePage that was causing issues that I "fixed" but caused regressions elsewhere).

KyleAMathews · 2019-02-17T15:16:56Z

What did you think of the onDeleteNode idea?

pieh · 2019-02-17T15:20:31Z

I didn't want to add new public API that there is probably no practical use for. But both adding onDeleteNode and faking API run would achieve same result - only question is do we want this new public API?

KyleAMathews · 2019-02-17T15:24:59Z

👍 yeah, it seems potentially useful but until we have concrete use cases for onDeleteNode, there's no point in adding. A special fake invocation of api-runner sounds like a great solution and can be swapped out easily for a real call if we ever add onDeleteNode.

pieh · 2019-02-17T17:52:02Z

I think this will also fix #10844 (which to me seems like same issue) - will validate now

pieh · 2019-02-17T19:44:13Z

After tests with https://github.com/rexxars/gatsby-node-update-bugs it fixes the node deletion not causing rerunning createPages / rerunning queries

But I uncovered bug with page-hot-reloader not cleaning up not re-created pages -

gatsby/packages/gatsby/src/bootstrap/page-hot-reloader.js

Line 60 in a420eb4

page.updatedAt < timestamp

timestamp check is not working because it compares number to date string - solution is to use Date.now() in

gatsby/packages/gatsby/src/bootstrap/page-hot-reloader.js

Line 48 in a420eb4

const timestamp = new Date().toJSON()

I'm checking out Bug #2 from the repro repo description right now - might we can kill all of those with one stone

KyleAMathews · 2019-02-18T06:58:19Z

Great catch on updatedAt! And yikes 😱 types would be nice in core. No idea why I used Date.now() for page.createdAt. Doing a quick look around, it looks like 85% of dates are ISO8601 and the remaining 15% are randomly unix timestamps.

KyleAMathews · 2019-02-18T06:59:13Z

Oh and verified that pages are added/removed on the dev 404 page — which wasn't updating smoothly for some reason.

KyleAMathews · 2019-02-18T07:21:58Z

Looking at @rexxars' (awesome!) repro — it looks like this fixes part of things — still seeing a problem where the title updates which causes the slug to change — for some reason the query for the new/changed page isn't run until I save the page component. I recall seeing an issue around changes to pageContext not triggering re-runs of GraphQL. Investigating...

KyleAMathews · 2019-02-18T13:06:52Z

#7375 (comment)

…be run again if page recreated

This reverts commit 890fb36.

This reverts commit 190625f.

KyleAMathews · 2019-02-20T10:37:11Z

Let's get this in with these fixes. Working on more fixes for https://github.com/rexxars/gatsby-node-update-bugs in #11897

wardpeet · 2019-02-20T10:49:33Z

packages/gatsby/src/bootstrap/page-hot-reloader.js

@@ -45,7 +46,7 @@ const runCreatePages = async () => {
    })
    .map(p => p.id)

-  const timestamp = new Date().toJSON()
+  const timestamp = Date.now()


new Date().toJSON() => "2019-02-20T10:47:41.468Z"
Date.now() => 1550659670910

I don't know how page.updatedAt looks so no idea if this is good or bad 😄

Yeah... for some reason I made a few types (like page) use unix timestamps instead of iso8601 :-(

So this check was failing before.

wardpeet · 2019-02-20T10:52:29Z

packages/gatsby/src/utils/api-runner-node.js

@@ -157,7 +157,7 @@ module.exports = async (api, args = {}, pluginSource) =>
    })

    // Check that the API is documented.
-    if (!apiList[api]) {
+    if (!apiList[api] && api !== `FAKE_API_CALL`) {


I can't see FAKE_API_CALL if you could point me in the right direction that would be awesome

Oh lemme add a comment on this. It's a bit weird :-D

I missed https://github.com/gatsbyjs/gatsby/pull/11831/files#diff-deed9a141e906a882e5f3b2d2c4e21aeR19 my bad :) it makes sense

wardpeet

Works like a charm tested it with blog-starter! 🎉

we also fix the following issue when recreating a page

warning Action createPage was called outside of its expected asynchronous lifecycle createPages in default-site-plugin.
Ensure that you return a Promise from createPages and are awaiting any asynchronous method invocations (like graphql or http requests).
For more info and debugging tips: see https://gatsby.app/sync-actions

File /home/ward/projects/tmp/my-blog-starter/gatsby-node.js:40:7
  38 |       const next = index === 0 ? null : posts[index - 1].node
  39 |
> 40 |       createPage({
     |       ^
  41 |         path: post.node.fields.slug,
  42 |         component: blogPost,
  43 |         context: {

KyleAMathews · 2019-02-20T11:16:26Z

Thanks for the review!

KyleAMathews · 2019-02-20T11:17:13Z

Do you mean we need to fix that warning? Like the new warning is popping up while developing a site?

KyleAMathews · 2019-02-20T11:17:27Z

And you create a new page?

wardpeet · 2019-02-20T12:23:21Z

no it's fixed :) Else I wouldn't approve 😄

When removing a blog post and when I try to rollback the blogpost I got an error. Your fix also fixes this.

…r edits (#12671) If in `gatsby-node.js` we add ```js exports.onCreatePage = ({ actions, page }) => { actions.createPage(page) } ``` Next time any node update, pages created by `gatsby-plugin-page-creator` will be deleted. This is regression introduced by fix in #11831 (that fixed the problem of pages never getting deleted) The problem is that when we do `createPage` by plugin/site that isn't implementing `createPagesStatefully` it will be marked to be deleted. This change keeps track if original API was `createPages` or `createPagesStatefully` by using `traceId` instead of directly checking what plugin created the page in the end. Fixes #12143

@pieh

…run and data is removed from pages) (gatsbyjs#11831) * fix: check for dirty pages when nodes are deleted (so queries are ru-run and data is removed from pages) * typo fix * Use fake API call as suggested by @pieh * better name * Fix comparision as page.updatedAt is a unix timestamp * Remove page when deleted from internal query running cache so it can be run again if page recreated * WIP * MORE WIP * Revert "MORE WIP" This reverts commit 890fb36. * Revert "WIP" This reverts commit 190625f. * Document FAKE_API_CALL

fix: check for dirty pages when nodes are deleted (so queries are ru-…

6d57b16

…run and data is removed from pages)

KyleAMathews requested a review from a team as a code owner February 17, 2019 06:50

typo fix

c3b9ec9

Use fake API call as suggested by @pieh

eb16fd8

better name

a9d9a2e

Fix comparision as page.updatedAt is a unix timestamp

7ffe5c7

KyleAMathews force-pushed the fix-delete-hot-reload branch from bc74458 to 7ffe5c7 Compare February 18, 2019 06:58

KyleAMathews added 5 commits February 18, 2019 14:15

Remove page when deleted from internal query running cache so it can …

5b5417f

…be run again if page recreated

WIP

190625f

MORE WIP

890fb36

Revert "MORE WIP"

dcfc73c

This reverts commit 890fb36.

Revert "WIP"

863acf1

This reverts commit 190625f.

wardpeet reviewed Feb 20, 2019

View reviewed changes

Document FAKE_API_CALL

516b42c

wardpeet approved these changes Feb 20, 2019

View reviewed changes

KyleAMathews merged commit 1fff689 into master Feb 20, 2019

KyleAMathews deleted the fix-delete-hot-reload branch February 20, 2019 11:16

KyleAMathews mentioned this pull request Feb 23, 2019

feat(gatsby): Move page component state & side effect handling to xstate #11897

Merged

2 tasks

wardpeet mentioned this pull request Feb 28, 2019

[V2] Hot reload doesn't work on pages deleted and created by onCreatePage #12143

Closed

KyleAMathews mentioned this pull request Mar 3, 2019

Pages created by "stateful" plugins can get deleted (e.g. those in src/pages) #12249

Closed

pieh mentioned this pull request Mar 19, 2019

fix(gatsby): keep track of pages created by stateful createPages after edits #12671

Merged

pieh mentioned this pull request Jul 26, 2019

feat(gatsby): enable richer error handling for unknown APIs #16105

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: check for dirty pages when nodes are deleted (so queries are ru-run and data is removed from pages) #11831

fix: check for dirty pages when nodes are deleted (so queries are ru-run and data is removed from pages) #11831

KyleAMathews commented Feb 17, 2019

KyleAMathews commented Feb 17, 2019

KyleAMathews commented Feb 17, 2019

KyleAMathews commented Feb 17, 2019

KyleAMathews commented Feb 17, 2019

KyleAMathews commented Feb 17, 2019

pieh commented Feb 17, 2019 •

edited

Loading

KyleAMathews commented Feb 17, 2019

pieh commented Feb 17, 2019

KyleAMathews commented Feb 17, 2019

pieh commented Feb 17, 2019

pieh commented Feb 17, 2019

KyleAMathews commented Feb 18, 2019

KyleAMathews commented Feb 18, 2019

KyleAMathews commented Feb 18, 2019

KyleAMathews commented Feb 18, 2019

KyleAMathews commented Feb 20, 2019

wardpeet Feb 20, 2019

KyleAMathews Feb 20, 2019

wardpeet Feb 20, 2019

KyleAMathews Feb 20, 2019

wardpeet Feb 20, 2019

wardpeet left a comment •

edited

Loading

KyleAMathews commented Feb 20, 2019

KyleAMathews commented Feb 20, 2019

KyleAMathews commented Feb 20, 2019

wardpeet commented Feb 20, 2019

fix: check for dirty pages when nodes are deleted (so queries are ru-run and data is removed from pages) #11831

fix: check for dirty pages when nodes are deleted (so queries are ru-run and data is removed from pages) #11831

Conversation

KyleAMathews commented Feb 17, 2019

KyleAMathews commented Feb 17, 2019

KyleAMathews commented Feb 17, 2019

KyleAMathews commented Feb 17, 2019

KyleAMathews commented Feb 17, 2019

KyleAMathews commented Feb 17, 2019

pieh commented Feb 17, 2019 • edited Loading

KyleAMathews commented Feb 17, 2019

pieh commented Feb 17, 2019

KyleAMathews commented Feb 17, 2019

pieh commented Feb 17, 2019

pieh commented Feb 17, 2019

KyleAMathews commented Feb 18, 2019

KyleAMathews commented Feb 18, 2019

KyleAMathews commented Feb 18, 2019

KyleAMathews commented Feb 18, 2019

KyleAMathews commented Feb 20, 2019

wardpeet Feb 20, 2019

Choose a reason for hiding this comment

KyleAMathews Feb 20, 2019

Choose a reason for hiding this comment

wardpeet Feb 20, 2019

Choose a reason for hiding this comment

KyleAMathews Feb 20, 2019

Choose a reason for hiding this comment

wardpeet Feb 20, 2019

Choose a reason for hiding this comment

wardpeet left a comment • edited Loading

Choose a reason for hiding this comment

KyleAMathews commented Feb 20, 2019

KyleAMathews commented Feb 20, 2019

KyleAMathews commented Feb 20, 2019

wardpeet commented Feb 20, 2019

pieh commented Feb 17, 2019 •

edited

Loading

wardpeet left a comment •

edited

Loading