[1.0] Query runner refactor #828

KyleAMathews · 2017-04-19T20:09:09Z

Running queries to date has been a fairly brute force operation. Let's make it faster/slimmer/testable/understandable.

cache queries so only re-run queries when a component changes if the query itself has changed
persist results of queries so only re-run the queries on page startup that have been invalidated
tests for the add-page-data-dependency action creator
only write routes files if there's been a path change
refactor query runner into multiple testable modules: query parser, routes writer, path invalidator, query runner.

gatsbybot · 2017-04-19T20:18:46Z

Deploy preview ready!

Built with commit d9d60d2

https://deploy-preview-828--gatsbyjs.netlify.com

gatsbybot · 2017-04-19T20:32:27Z

Deploy preview ready!

Built with commit 6560744

https://deploy-preview-828--gatsbyjs.netlify.com

…ities

gatsbybot · 2017-04-21T00:48:08Z

Deploy preview ready!

Built with commit 836841b

https://deploy-preview-828--gatsbygram.netlify.com

KyleAMathews · 2017-04-21T00:53:32Z

So I think this refactor is broadly correct. My main concern is with how sloppy some of the event chaining logic is. Not going to worry about it much now as this stuff will get worked over again in the future.

fabien0102

Big diff! Good works ;) just some styles comments/questions and maybe a bad copy/past find

Note: I haven't run the project on this branch for instant

fabien0102 · 2017-04-21T07:54:54Z

packages/gatsby/lib/bootstrap/index.js

@@ -305,8 +308,8 @@ module.exports = async (program: any) => {

  console.log(`created js pages`)

-  return new Promise(resolve => {
-    queryRunner(thing => {
+  const finishPromise = new Promise(resolve => {


This const it's really useless and finishPromise is a very strange wording ^^ (what this Promise represent? A variable name never should to be generic)

http://codebuild.blogspot.de/2012/02/15-best-practices-of-variable-method.html 😉

Oh oops that was leftover from when I was experimenting with something. Fixed to just returning the promise.

fabien0102 · 2017-04-21T08:02:13Z

packages/gatsby/lib/query-runner/page-query-runner.js

+ * - Watch for when a page's query is invalidated and re-run it.
+ ***/
+
+const _ = require("lodash")


Just a general style reflection (es6 module vs require):

Sometime we use import ... from ... sometime const ... = require(), I think es6 module style is better for performance/readibility but more important I think we need to make a choice! (I always ask to myself "what is better for this file?")

So, what we choose?

ref: https://medium.com/the-node-js-collection/an-update-on-es6-modules-in-node-js-42c958b890c

Same fact from lodash, we have four import patterns:

const _ = require("lodash")

const {get, forEach} = require("lodash")

import _ from "lodash"

import {get, forEach} from "lodash"

The general rule I've arrived at (not universally followed) is to use commonjs for node stuff and imports for browser. I write enough "raw" node.js code that getting tripped up by es6 stuff not working when I'm not compiling the code gets really annoying.

For lodash, I just always import the underscore as I dislike going back and forth to change which exact methods I'm importing. When writing browser code, I then just add the webpack/babel lodash plugins to optimize the builds. Which speaking of, it'd be good to get a Gatsby plugin for this.

👍

Maybe it can be a good idea to add this "coding guidelines" into CONTRIBUTING.md (like https://github.com/lodash/lodash/blob/master/.github/CONTRIBUTING.md#coding-guidelines)

fabien0102 · 2017-04-21T08:08:25Z

packages/gatsby/lib/query-runner/page-query-runner.js

+    })
+
+    // Tell everyone who cares that we're done.
+    callbacks.forEach(cb => cb())


I don't try, but I think it's a good idea to add callbacks = [] after (to avoid multiple callbacks calls)

Callbacks (the array) isn't ever used again after this... which isn't obvious I agree. But this again points to the weakness of how I've been writing the (I'm not sure what's the right way to describe this) event based boot system.

I was thinking yesterday that SystemD might actually be a good model to follow since Gatsby is kind of an OS. Will read up more on that http://0pointer.de/blog/projects/systemd.html

If there's any other programming architectures you can think of for booting up an event-based system, lemme know.

I think we can have a look into ionic-dev-tools , they've got many of our problematics (live-reload, watch, build, notifications…) and their code is very clean ;)

fabien0102 · 2017-04-21T08:14:57Z

packages/gatsby/lib/query-runner/query-extractor.js

+  })
+
+  // Trim whitespace
+  if (query) {


You can use lodash method (most robut => works with null)

query = _.trim(query)

fabien0102 · 2017-04-21T08:19:47Z

packages/gatsby/lib/query-runner/query-watcher.js

+
+const debounceNewPages = _.debounce(() => {
+  store.dispatch({
+    type: `BOOTSTRAP_STAGE`,


It can be good to have a constant.js or anything else to have a list with all possible actions (to avoid any collision).

Another good practice is to have an actionCreator even if the action is very simple (with this pattern, all actions are centralized, so it's easier to maintain)

Yeah, this is my first Redux project so I've heard of doing that but have been skipping it so far to keep moving quicker :-) But yeah, let's refactor things soon to use a constants.js

I don't really like the pattern constants.js (you have 3 files to update for 1 action… so yeah, you moving slower) my best compromise to have flexibility and maintabiliy is to merge actionsCreator & constants (an example into my redux sandbox)

fabien0102 · 2017-04-21T08:26:51Z

packages/gatsby/lib/query-runner/route-writer.js

+      // If there's not an index page, just pick the one with the shortest path.
+      // Probably a bad heuristic.
+      if (!indexPage) {
+        indexPage = _.first(_.sortBy(pages, page => page.path.length))


Just a style suggestion:

_(pages).sortBy(page => page.path.length).first()

Quite same, but you retrieve the natural reading way.

Oh nice! Haven't used this style before but I like it.

Chaining with lodash, I just love this notation 😄 Just some subtilities with lodashWrapper

I recommend this talk if you like it:
http://www.nicoespeon.com/en/2015/06/functions-chaining-composition-lodash-underscore/

fabien0102 · 2017-04-21T08:33:21Z

packages/gatsby/lib/redux/index.js

@@ -41,6 +38,25 @@ if (process.env.NODE_ENV === `test` || process.env.NODE_ENV === `production`) {
  )
 }

+// Persist state.
+const saveState = _.debounce(state => {


Nice! It's better here ;)

fabien0102 · 2017-04-21T08:38:53Z

packages/gatsby/lib/redux/reducers/page-components.js

+module.exports = (state = {}, action) => {
+  switch (action.type) {
+    case "ADD_PAGE_COMPONENT":
+      if (!state[action.payload.componentPath]) {


This kind of pattern affraid me (a bad past experience this object[a.b.c]…) It's proofer to use the lodash get (it's will not break if you have an undefined somewhere 😃 )

So for this => get(state, 'action.payload.componentPath')

(I know, normally we never should have undefined in payload, it's just a protection, it's not a big cost and really improve an application stability)

fabien0102 · 2017-04-21T08:42:13Z

packages/gatsby/lib/redux/reducers/page-data-dependencies.js

      }

      return state
+    case "REMOVE_PAGES_DATA_DEPENDENCIES":
+      Object.keys(state.nodes).forEach(nodeId => {


mmmm, http://devdocs.io/lodash~4/index#mapValues ?

Oooo nice! Much cleaner.

fabien0102 · 2017-04-21T08:42:45Z

packages/gatsby/lib/redux/reducers/page-data-dependencies.js

+        )
+      })
+      Object.keys(state.connections).forEach(connection => {
+        state.nodes[connection] = _.difference(


I think is state.connections[connection] here

KyleAMathews added 3 commits April 19, 2017 12:57

Add the plugin creating the action to the action to ease debugging

6d54f28

Fix bugs in adding page dependencies plus only add if new

d9d60d2

Upgrade Prettier to 1.2.1 and re-run format

c0bba96

KyleAMathews added 4 commits April 19, 2017 13:44

Up sites to latest canary

bf2dd32

Move route writer to own module and only write when pages change

d4b40ae

Add new custom bound action creator which filters out unneeded actions

b6eb2cc

Split the query-runner module into 5 modules with clearer responsibil…

89d880e

…ities

KyleAMathews force-pushed the query-runner-refactor branch from 8756144 to 89d880e Compare April 21, 2017 00:29

Remove requires to now missing file

b66dd8d

KyleAMathews requested a review from fabien0102 April 21, 2017 00:51

fabien0102 reviewed Apr 21, 2017

View reviewed changes

KyleAMathews added 2 commits April 21, 2017 08:54

Address feedback

6560744

Update dependencies

836841b

KyleAMathews merged commit f850a6c into 1.0 Apr 21, 2017

KyleAMathews deleted the query-runner-refactor branch April 21, 2017 17:01

KyleAMathews changed the title ~~[1.0 WIP Query runner refactor~~ [1.0] Query runner refactor Apr 21, 2017

fabien0102 mentioned this pull request Apr 26, 2017

[1.0] Plugin packaging file structure #858

Closed

snyk-bot mentioned this pull request Dec 28, 2022

[Snyk] Fix for 1 vulnerabilities tejzpr/gatsby#56

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1.0] Query runner refactor #828

[1.0] Query runner refactor #828

KyleAMathews commented Apr 19, 2017 •

edited

Loading

gatsbybot commented Apr 19, 2017

gatsbybot commented Apr 19, 2017 •

edited

Loading

gatsbybot commented Apr 21, 2017 •

edited

Loading

KyleAMathews commented Apr 21, 2017

fabien0102 left a comment

fabien0102 Apr 21, 2017

KyleAMathews Apr 21, 2017

fabien0102 Apr 21, 2017

fabien0102 Apr 21, 2017

KyleAMathews Apr 21, 2017

fabien0102 Apr 21, 2017

fabien0102 Apr 21, 2017

KyleAMathews Apr 21, 2017

fabien0102 Apr 21, 2017

fabien0102 Apr 21, 2017

KyleAMathews Apr 21, 2017

fabien0102 Apr 21, 2017

KyleAMathews Apr 21, 2017

fabien0102 Apr 21, 2017

fabien0102 Apr 21, 2017

KyleAMathews Apr 21, 2017

fabien0102 Apr 21, 2017

fabien0102 Apr 21, 2017

fabien0102 Apr 21, 2017

KyleAMathews Apr 21, 2017

fabien0102 Apr 21, 2017

KyleAMathews Apr 21, 2017

fabien0102 Apr 21, 2017

[1.0] Query runner refactor #828

[1.0] Query runner refactor #828

Conversation

KyleAMathews commented Apr 19, 2017 • edited Loading

gatsbybot commented Apr 19, 2017

gatsbybot commented Apr 19, 2017 • edited Loading

gatsbybot commented Apr 21, 2017 • edited Loading

KyleAMathews commented Apr 21, 2017

fabien0102 left a 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

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

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

KyleAMathews commented Apr 19, 2017 •

edited

Loading

gatsbybot commented Apr 19, 2017 •

edited

Loading

gatsbybot commented Apr 21, 2017 •

edited

Loading