refactor(gatsby): Page dependency resolver #9732

Moocar · 2018-11-06T04:27:37Z

Anywhere in Gatsby where a graphql resolver returns a node, we need to declare a dependency from the node to the page context.path. This code is littered throughout schema. Eventually, it would be great to figure out an alternative way to declare these dependencies, since causing side effects in query resolvers isn't ideal, but in the mean time, this PR cleans up the code a bit by introducing a resolver middleware called pageDependencyResolver. It executes a given resolver and then creates a page dependency.

Another alternative would be to use graphql-middleware to run this on all field resolvers. Perhaps another day.

pieh · 2018-11-08T13:49:14Z

Trying to build gatsbyjs.org with this changes I'm getting theses errors:

error
The GraphQL query from /Users/misiek/dev/gatsby/www/src/pages/docs/actions.js failed.

Errors:
  Cannot read property 'id' of undefined

  GraphQL request (40:5)
  39:     }
  40:     description {
          ^
  41:       childMarkdownRemark {
  ,Cannot read property 'id' of undefined

  GraphQL request (40:5)
  39:     }
  40:     description {
          ^
  41:       childMarkdownRemark {
  ,Cannot read property 'id' of undefined

  GraphQL request (40:5)
  39:     }
  40:     description {
          ^
  41:       childMarkdownRemark {
  ,Cannot read property 'id' of undefined

  GraphQL request (40:5)
  39:     }
  40:     description {
          ^
  41:       childMarkdownRemark {
  ,Cannot read property 'id' of undefined

  GraphQL request (40:5)
  39:     }
  40:     description {
          ^
  41:       childMarkdownRemark {
  ,Cannot read property 'id' of undefined

  GraphQL request (40:5)
  39:     }
  40:     description {
          ^
  41:       childMarkdownRemark {
  ,Cannot read property 'id' of undefined

  GraphQL request (40:5)
  39:     }
  40:     description {
          ^
  41:       childMarkdownRemark {
  ,Cannot read property 'id' of undefined

  GraphQL request (40:5)
  39:     }
  40:     description {
          ^
  41:       childMarkdownRemark {
  ,Cannot read property 'id' of undefined

  GraphQL request (40:5)
  39:     }
  40:     description {
          ^
  41:       childMarkdownRemark {
  ,Cannot read property 'id' of undefined

  GraphQL request (40:5)
  39:     }
  40:     description {
          ^
  41:       childMarkdownRemark {
  ,Cannot read property 'id' of undefined

  GraphQL request (40:5)
  39:     }
  40:     description {
          ^
  41:       childMarkdownRemark {
  ,Cannot read property 'id' of undefined

  GraphQL request (40:5)
  39:     }
  40:     description {
          ^
  41:       childMarkdownRemark {
  ,Cannot read property 'id' of undefined

  GraphQL request (40:5)
  39:     }
  40:     description {
          ^
  41:       childMarkdownRemark {
  ,Cannot read property 'id' of undefined

  GraphQL request (40:5)
  39:     }
  40:     description {
          ^
  41:       childMarkdownRemark {
  ,Cannot read property 'id' of undefined

  GraphQL request (40:5)
  39:     }
  40:     description {
          ^
  41:       childMarkdownRemark {
  ,Cannot read property 'id' of undefined

  GraphQL request (40:5)
  39:     }
  40:     description {
          ^
  41:       childMarkdownRemark {
  ,Cannot read property 'id' of undefined

  GraphQL request (40:5)
  39:     }
  40:     description {
          ^
  41:       childMarkdownRemark {
  ,Cannot read property 'id' of undefined

  GraphQL request (40:5)
  39:     }
  40:     description {
          ^
  41:       childMarkdownRemark {
  ,Cannot read property 'id' of undefined

  GraphQL request (40:5)
  39:     }
  40:     description {
          ^
  41:       childMarkdownRemark {
  ,Cannot read property 'id' of undefined

  GraphQL request (40:5)
  39:     }
  40:     description {
          ^
  41:       childMarkdownRemark {
  ,Cannot read property 'id' of undefined

  GraphQL request (40:5)
  39:     }
  40:     description {
          ^
  41:       childMarkdownRemark {
  ,Cannot read property 'id' of undefined

  GraphQL request (40:5)
  39:     }
  40:     description {
          ^
  41:       childMarkdownRemark {
  ,Cannot read property 'id' of undefined

  GraphQL request (40:5)
  39:     }
  40:     description {
          ^
  41:       childMarkdownRemark {
  ,Cannot read property 'id' of undefined

  GraphQL request (40:5)
  39:     }
  40:     description {
          ^
  41:       childMarkdownRemark {
  ,Cannot read property 'id' of undefined

  GraphQL request (40:5)
  39:     }
  40:     description {
          ^
  41:       childMarkdownRemark {
  ,Cannot read property 'id' of undefined

  GraphQL request (40:5)
  39:     }
  40:     description {
          ^
  41:       childMarkdownRemark {
  ,Cannot read property 'id' of undefined

  GraphQL request (40:5)
  39:     }
  40:     description {
          ^
  41:       childMarkdownRemark {
  ,Cannot read property 'id' of undefined

  GraphQL request (51:7)
  50:       default
  51:       description {
            ^
  52:         childMarkdownRemark {
  ,Cannot read property 'id' of undefined

  GraphQL request (40:5)
  39:     }
  40:     description {
          ^
  41:       childMarkdownRemark {
  ,Cannot read property 'id' of undefined

  GraphQL request (40:5)
  39:     }
  40:     description {
          ^
  41:       childMarkdownRemark {
  ,Cannot read property 'id' of undefined

  GraphQL request (40:5)
  39:     }
  40:     description {
          ^
  41:       childMarkdownRemark {

URL path:
  /docs/actions/
Context:
  {}
Plugin:
  none
Query:
  query usersMisiekDevGatsbyWwwSrcPagesDocsActionsJs807554310 {
    file(relativePath: {eq: "gatsby/src/redux/actions.js"}) {
      childrenDocumentationJs {
        name
        ...FunctionList
      }
    }
  }

  fragment FunctionList on DocumentationJs {
    name
    description {
      childMarkdownRemark {
        html
      }
    }
    returns {
      type {
        name
        type
        elements {
          name
          type
        }
      }
      description {
        childMarkdownRemark {
          html
        }
      }
    }
    examples {
      highlighted
    }
    params {
      name
      type {
        name
      }
      description {
        childMarkdownRemark {
          html
        }
      }
      properties {
        name
        type {
          name
        }
        default
        description {
          childMarkdownRemark {
            html
          }
        }
        properties {
          name
          type {
            name
          }
          description {
            childMarkdownRemark {
              html
            }
          }
        }
      }
    }
  }

I will do some research to hopefully pinpoint what change triggers this

packages/gatsby/src/schema/infer-graphql-type.js

pieh · 2018-11-08T16:06:52Z

packages/gatsby/src/redux/actions/add-page-dependency.js

+    if (path) {
+      const asArray = _.isArray(result) ? result : [result]
+      for (const node of asArray) {
+        // using module.exports here so it can be mocked


I think we should split those into separate module/files if we want to mock it - this feels pretty hacky

Yeah, I hated myself for doing it. But I couldn't figure out any other way to mock it properly (and had wasted hours on it by that point). Another module sounds like a good idea

pieh · 2018-11-08T16:07:48Z

packages/gatsby/src/redux/actions/add-page-dependency.js

+      const asArray = _.isArray(result) ? result : [result]
+      for (const node of asArray) {
+        // using module.exports here so it can be mocked
+        module.exports.createPageDependency({


we need to check if node is defined here - null can be passed and in that case we definitely don't want to try to create dependency as it will fail on accessing node.id

Moocar · 2018-11-08T20:59:35Z

@pieh added check for node being defined, and moved pageDependencyResolver into its own module.

pieh

🙏

Anywhere in Gatsby where a graphql resolver returns a node, we need to declare a dependency from the node to the page `context.path`. This code is littered throughout `schema`. Eventually, it would be great to figure out an alternative way to declare these dependencies, since causing side effects in query resolvers isn't ideal, but in the mean time, this PR cleans up the code a bit by introducing a resolver middleware called `pageDependencyResolver`. It executes a given resolver and then creates a page dependency. Another alternative would be to use [graphql-middleware](https://github.com/prisma/graphql-middleware) to run this on all field resolvers. Perhaps another day.

Moocar added 2 commits November 6, 2018 15:12

add page dependency resolver

ed17fa2

misc fixes

f571aad

Moocar requested a review from a team as a code owner November 6, 2018 04:27

pieh reviewed Nov 8, 2018

View reviewed changes

packages/gatsby/src/schema/infer-graphql-type.js Show resolved Hide resolved

pieh reviewed Nov 8, 2018

View reviewed changes

Moocar added 2 commits November 9, 2018 06:49

Merge branch 'master' into page-dependency-resolver

06772e6

added node defined check in pageDependencyResolver

829a32d

pieh approved these changes Nov 8, 2018

View reviewed changes

pieh merged commit c2c50a8 into gatsbyjs:master Nov 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(gatsby): Page dependency resolver #9732

refactor(gatsby): Page dependency resolver #9732

Moocar commented Nov 6, 2018

pieh commented Nov 8, 2018

pieh Nov 8, 2018

Moocar Nov 8, 2018

pieh Nov 8, 2018

Moocar commented Nov 8, 2018 •

edited

pieh left a comment

refactor(gatsby): Page dependency resolver #9732

refactor(gatsby): Page dependency resolver #9732

Conversation

Moocar commented Nov 6, 2018

pieh commented Nov 8, 2018

pieh Nov 8, 2018

Choose a reason for hiding this comment

Moocar Nov 8, 2018

Choose a reason for hiding this comment

pieh Nov 8, 2018

Choose a reason for hiding this comment

Moocar commented Nov 8, 2018 • edited

pieh left a comment

Choose a reason for hiding this comment

Moocar commented Nov 8, 2018 •

edited