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

KyleAMathews · 2019-02-19T15:34:30Z

Internally within Gatsby, some of our most complex logic is around creating and managing page components — there's quite a few states a page component can be in e.g. while bootstrapping, extracting queries, running queries for its pages, etc.

The code for handling this has been some of the most annoying and bug prone code to work in.

XState has been on list of things to try in core for quite a while as it's perfect for these sorts of things. @m-allanson did a small PR adding it for ludicrious mode last year #4555 and it's been problem free there.

Visualization

TODOs

extract machine to separate directory
add tests for critical flows

Follow up questions/work

Model individual pages
- Fix bug #2b in https://github.com/rexxars/gatsby-node-update-bugs the old page is now deleted due to fix: check for dirty pages when nodes are deleted (so queries are ru-run and data is removed from pages) #11831 — to fix running the query again, we need to model new pages
- Fix this bug with pageContext changing not triggering rebuilds
- Track errors per page
- pages run their own queries instead of the parent pageComponent handling that — pageComponent tells them the query has changed and they then message the query runner to re-run their query.
how to model static queries? It looks like they're getting run every time their component changes.
would xstate be helpful for data-driven state changes? Presumably as a data change could happen at the same time as a query is being extracted. So we should then send action when a page is dirtied.
it seems like handling error reporting in machines would work really well
how do we want to make private actions truly private? Move to different file? Mark with underscore and filter out from passing to plugins?

… major relay-compilier update

…while bootstrapping + initiate extracting queries from machine when new page component is created

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

packages/gatsby/src/internal-plugins/query-runner/graphql-errors.js

@@ -55,21 +55,22 @@ function formatError(message: string, filePath: string, codeFrame: string) {
 }

 function extractError(error: Error): { message: string, docName: string } {
-  const docRegex = /Invariant Violation: (RelayParser|GraphQLParser): (.*). Source: document `(.*)` file:/g


This regex broke when we moved to Relay Compiler v2.

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

packages/gatsby/src/redux/actions.js

+ * Report that a query has been extracted from a component. Used by
+ * query-compilier.js.
+ *
+ * @param {Object} $0


I want to use actions more for internal communication but don't want them all to show up here (https://www.gatsbyjs.org/docs/actions/)

Thinking we could start marking actions as private to get around this?

I'm all for using actions more (and less event emitting). Can we move them to the actions folder that already exists (but only has the page dependencies stuff), and export a subset that can be used in node APIs?

Hah, that's pretty much what we talked about yesterday. One idea is to have public-actions and private-actions files. Other could be, have extra export here:

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

Lines 1186 to 1189 in 9e92a39

/**

* All action creators wrapped with a dispatch. - *DEPRECATED*

*/

exports.boundActionCreators = bindActionCreators(actions, store.dispatch)

const boundActionCreators = bindActionCreators(actions, store.dispatch) exports.boundActionCreators = boundActionCreators /** * This will be used in `api-runner-node.js` */ exports.publicBoundActionCreators = _.pick(boundActionCreators, [ `createNode`, `createPage`, // and all other actions that should be public ])

But it's not in scope for this PR

I like just using pick :)

wardpeet · 2019-02-20T12:00:33Z

packages/gatsby/src/redux/reducers/components.js

+)
+
+const services = new Map()
+let programStatus = `BOOTSTRAPPING`


we probably want to move to an enum.

…nt machine

…ies if the page query changes

stefanprobst · 2019-02-24T19:51:54Z

packages/gatsby/src/internal-plugins/internal-data-bridge/gatsby-node.js

@@ -157,3 +157,7 @@ emitter.on(`DELETE_PAGE`, action => {
  const node = getNode(nodeId)
  boundActionCreators.deleteNode({ node })
 })
+
+emitter.on(`BOOTSTRAP_FINISHED`, () =>
+  boundActionCreators.setProgramStatus(`BOOTSTRAP_FINISHED`)


Should we move this here?

Yeah, that'd make more sense 👍

KyleAMathews · 2019-02-25T15:37:01Z

packages/gatsby/src/internal-plugins/query-runner/page-query-runner.js

@@ -42,13 +42,14 @@ const runQueries = async () => {

  // Find ids without data dependencies (i.e. no queries have been run for
  // them before) and run them.
-  const cleanIds = findIdsWithoutDataDependencies()


probably need to restore this? For now

…queries in a machine as well)

…e has been created and run its query

DSchau · 2019-03-13T20:43:13Z

@KyleAMathews @pieh didn't have as much time today as I would've liked, but got through some of these. Definitely a few issues thus far that need fixing.

I'll look into actually fixing these tomorrow afternoon.

Here's what I found thus far:

Issues

4.iv) Deleting a page seems to hard fail

Add a page (e.g. pages/page-2.js)
Delete the page

TypeError: expected a string

- index.js:12 normalizePath
[xstate-example]/[normalize-path]/index.js:12:11
  - components.js:131 module.exports
    [xstate-example]/[gatsby]/dist/redux/reducers/components.js:131:40
  
  - redux.js:451 combination
    [xstate-example]/[redux]/lib/redux.js:451:29
  
  - redux.js:211 dispatch
    [xstate-example]/[redux]/lib/redux.js:211:22
  
  - index.js:78 action
    [xstate-example]/[gatsby]/dist/redux/index.js:78:91
  
  - redux.js:468 
    [xstate-example]/[redux]/lib/redux.js:468:12
  
  - api-runner-node.js:55 doubleBoundActionCreators.(anonymous function).args
    [xstate-example]/[gatsby]/dist/utils/api-runner-node.js:55:13

Recovering from an Error
- Used throw new Error in a function body
- Hot reload failed
- Removed throw new error, error still appears

Misc. / Possibly Unrelated

StaticQuery cannot be changed and hot reloaded (or really used at all in the development server)

Error: The result of this StaticQuery could not be fetched.
Hot reloading seems to log things at least twice?
- This may be unrelated to this PR
- Add a console.log in a page/template/component and it logs (at least) twice

pieh · 2019-03-15T19:29:04Z

4.iv) Deleting a page seems to hard fail

Add a page (e.g. pages/page-2.js)

Delete the page

Should be fixed in 36bda2e

StaticQuery cannot be changed and hot reloaded (or really used at all in the development server)

Error: The result of this StaticQuery could not be fetched.

This is bigger one - with approach change in here with emitting PAGE_COMPONENT_CHANGE action and not calling to extract directly we essentially we will never trigger query runs for static queries changes (edits/adding new static queries or removing old ones). I'm not exactly sure why query extraction was moved to be called by page-component machine and why it needs to be tracked there. I will open PR against this branch so it can be reviewed separately

KyleAMathews · 2019-03-16T00:37:36Z

Gotta another plane ride so I can work on the static query stuff.

…ant changes to static queries weren't being run

…l successfully extracts queries so we can know to leave babe error state

KyleAMathews · 2019-03-16T03:53:44Z

Made good progress on the train. Fixed the static query bug (by eliminating tracking extraction in the page component machine as you suggested @pieh).

I filled out the test plan a bunch more and from my manual tests, everything is currently working 🎉

Compile during bootstrap
1. compile all queries during bootstrap
2. No queries are compiled until the appropriate place in bootstrap
Run queries during bootstrap
1. Run all queries when there’s a clean cache
2. Run all queries for pages that have changed since the previous run:
  1. changed data
  2. changed queries
  3. queries we don’t have any tracked data for (so unknown state)
3. No queries are run until the appropriate place in the bootstrap
4. No queries are re-run (unless data/query changes) after the initial point in the bootstrap (implying that we know for sure at that point all queries which need run)
5. [not worked if page query isn’t run] Each page is tracked
6. Static Queries are run when they change
Compile during development
1. Queries should be re-compiled whenever a JS file inside src/ changes
2. Queries should be updated inside page component machine
Run queries during development
1. When the query changes
2. When data relied on by a query changes
3. When a new page added
  1. with an existing page template
  2. with a new page template
4. When an existing page component is deleted and then re-added (the old page data dependencies should be deleted so adding the new page runs everything fresh)
5. When the data a page depends on is deleted and re-added, the page is recreated correctly and its query run again
Error states handling (how do we help people resolve errors/warnings)
1. JS & GraphQL errors in a JS file block their page queries from running
2. Once the error is resolved, if the query has changed, queries are run again. If it’s a JS error, queries aren’t re-run

KyleAMathews · 2019-03-16T05:15:06Z

Checked and https://github.com/rexxars/gatsby-node-update-bugs are also fixed

jlengstorf · 2019-03-22T01:24:43Z

Niiiice. Let me know when you've got an alpha for this and I'll run my sites against it. Super stoked to see this!

…spond the same way

ChristopherBiscardi · 2019-03-22T01:43:14Z

same. Happy to run the mdx examples and such against it too.

KyleAMathews · 2019-03-22T02:49:20Z

Sweet thanks! Release gatsby@2.1.5-alpha.72

jlengstorf · 2019-03-22T21:01:36Z

[removed] whatever is happening is not due to this alpha. Apparently something else broke in my setup. 😕

Moocar · 2019-03-24T04:43:50Z

Looking forward to this one. I've been working on a follow up PR that eliminates some of the eventing and global state by making the query runner queue constructable. I'll create a PR once this is merged. https://github.com/gatsbyjs/gatsby/compare/xstate-components...Moocar:query-runner-refactor?expand=1

KyleAMathews · 2019-03-26T01:42:31Z

Tests passed — manually tested this on a number of sites + my manual checklist above.

Merging and publishing.

I'll create an issue for turning the checklist into automated tests.

DSchau · 2019-03-26T01:43:20Z

@KyleAMathews 🎉 awesome news!

KyleAMathews · 2019-03-26T01:50:47Z

Successfully published:
 - gatsby@2.2.12

Inital work to move component query handling to xstate + fix bug with…

d61316e

… major relay-compilier update

KyleAMathews requested a review from a team as a code owner February 19, 2019 15:34

KyleAMathews changed the title ~~Move component state/actions to xstate~~ [WIP] Move component state/actions to xstate Feb 19, 2019

KyleAMathews added 2 commits February 19, 2019 15:32

Expand model to include we don't want to immediately extract queries …

5db0b2c

…while bootstrapping + initiate extracting queries from machine when new page component is created

Rearrange states so more in order of possible events

2abcb2c

KyleAMathews mentioned this pull request Feb 20, 2019

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

Merged

Merge remote-tracking branch 'origin/master' into xstate-components

799391f

KyleAMathews commented Feb 20, 2019

View reviewed changes

wardpeet reviewed Feb 20, 2019

View reviewed changes

KyleAMathews added 7 commits February 20, 2019 04:08

Document two program statuses

bbbd20b

Move responsibility for running queries for pages to the page compone…

5f53d33

…nt machine

Handle page components changing

d8fb163

Let state machine know when a query has been run and only re-run quer…

97226e6

…ies if the page query changes

Handle babel error state when extracing queries

9347c2f

Make name more explicit

d242435

Ignore static query results

a124367

KyleAMathews mentioned this pull request Feb 24, 2019

Don't silently process.exit #12042

Closed

stefanprobst reviewed Feb 24, 2019

View reviewed changes

KyleAMathews added 3 commits February 25, 2019 07:02

Clarify comment

aaf8bf3

empty strings are falsy 🤦‍♂️

53eea1c

Handled in last commit

0e4d4df

KyleAMathews commented Feb 25, 2019

View reviewed changes

KyleAMathews added 7 commits February 25, 2019 07:57

Only handle clean ids for static queries (for now until model static …

df226de

…queries in a machine as well)

In hacky/temp way (until modeling pages directly), check if a new pag…

6f60428

…e has been created and run its query

Mark new actions as private

8631558

Move action to bootstrap per @stefanprobst's suggestion

e5525b4

Extract page component machine into new 'machines' directory

647b2dd

Fix check that there's a query in a non-page component

c3bb339

Add xstate as a dependency

3e0ddcf

Merge remote-tracking branch 'upstream/master' into xstate-components

c299ae6

DELETE_PAGE action use component not componentPath

36bda2e

KyleAMathews added 2 commits March 16, 2019 11:44

Page components shouldn't control extraction — moving that to them me…

1adc63d

…ant changes to static queries weren't being run

Now that we're not tracking query extracting, add event for when babe…

7932e8e

…l successfully extracts queries so we can know to leave babe error state

Merge remote-tracking branch 'origin/master' into xstate-components

f4bb7c2

Merge remote-tracking branch 'origin/master' into xstate-components

20fcf49

KyleAMathews requested a review from a team as a code owner March 22, 2019 01:21

Move graphql/babel event handling to top-level as we should always re…

03f4b37

…spond the same way

KyleAMathews added 2 commits March 21, 2019 19:43

Move action call to after findGraphQLTags as we cache babel extraction

8dfe393

trivial change

cedc761

KyleAMathews added 4 commits March 25, 2019 16:55

Merge remote-tracking branch 'origin/master' into xstate-components

b3fd97b

yarn.lock change

83eedb2

Trivial change so can publish

76475f5

Update from master

67ef857

KyleAMathews merged commit 91086d4 into master Mar 26, 2019

KyleAMathews deleted the xstate-components branch March 26, 2019 01:42

Moocar mentioned this pull request Mar 27, 2019

refactor(gatsby): enable running of static/page queries separately #12891

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

KyleAMathews commented Feb 19, 2019 •

edited by janosh

KyleAMathews Feb 20, 2019

KyleAMathews Feb 20, 2019

stefanprobst Feb 26, 2019

pieh Feb 26, 2019

wardpeet Feb 27, 2019

wardpeet Feb 20, 2019

stefanprobst Feb 24, 2019

KyleAMathews Feb 25, 2019

KyleAMathews Feb 25, 2019

DSchau commented Mar 13, 2019 •

edited

pieh commented Mar 15, 2019

KyleAMathews commented Mar 16, 2019

KyleAMathews commented Mar 16, 2019

KyleAMathews commented Mar 16, 2019

jlengstorf commented Mar 22, 2019

ChristopherBiscardi commented Mar 22, 2019

KyleAMathews commented Mar 22, 2019

jlengstorf commented Mar 22, 2019 •

edited

Moocar commented Mar 24, 2019 •

edited

KyleAMathews commented Mar 26, 2019

DSchau commented Mar 26, 2019

KyleAMathews commented Mar 26, 2019

	/**
	* All action creators wrapped with a dispatch. - DEPRECATED
	*/
	exports.boundActionCreators = bindActionCreators(actions, store.dispatch)

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

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

Conversation

KyleAMathews commented Feb 19, 2019 • edited by janosh

Visualization

TODOs

Follow up questions/work

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DSchau commented Mar 13, 2019 • edited

Issues

Misc. / Possibly Unrelated

pieh commented Mar 15, 2019

KyleAMathews commented Mar 16, 2019

KyleAMathews commented Mar 16, 2019

KyleAMathews commented Mar 16, 2019

jlengstorf commented Mar 22, 2019

ChristopherBiscardi commented Mar 22, 2019

KyleAMathews commented Mar 22, 2019

jlengstorf commented Mar 22, 2019 • edited

Moocar commented Mar 24, 2019 • edited

KyleAMathews commented Mar 26, 2019

DSchau commented Mar 26, 2019

KyleAMathews commented Mar 26, 2019

KyleAMathews commented Feb 19, 2019 •

edited by janosh

DSchau commented Mar 13, 2019 •

edited

jlengstorf commented Mar 22, 2019 •

edited

Moocar commented Mar 24, 2019 •

edited