chore(gatsby-plugin-page-creator): Standardize error reporting text (g…

…atsbyjs#26373) * chore(gatsby-plugin-page-creator): Standardize error reporting to simplify analytics * fix tests and stop crashing at times and crash the build at other times * Fix lint * Thread reporter through functions instead of importing from gatsby-cli * fix lint Co-authored-by: Sidhartha Chatterjee <me@sidharthachatterjee.com>
MaxMood96 · Aug 12, 2020 · 2771534 · 2771534
1 parent 3c19438
commit 2771534
Show file tree

Hide file tree

Showing 13 changed files with 264 additions and 160 deletions.
diff --git a/packages/gatsby-plugin-page-creator/src/__tests__/collection-extract-query-string.ts b/packages/gatsby-plugin-page-creator/src/__tests__/collection-extract-query-string.ts
@@ -1,6 +1,10 @@
 import sysPath from "path"
 import fs from "fs-extra"
 import { collectionExtractQueryString } from "../collection-extract-query-string"
+import reporter from "gatsby-cli/lib/reporter"
+
+jest.mock(`gatsby-cli/lib/reporter`)
+
 // This makes the tests work on windows properly
 const createPath = (path: string): string => path.replace(/\//g, sysPath.sep)
 
@@ -24,7 +28,8 @@ describe(`collectionExtractQueryString`, () => {
       `)
 
     const query = await collectionExtractQueryString(
-      createPath(`src/pages/{Product.name}`)
+      createPath(`src/pages/{Product.name}`),
+      reporter
     )
 
     expect(query).toMatchInlineSnapshot(`"{allProduct{nodes{name,id}}}"`)
@@ -39,24 +44,28 @@ describe(`collectionExtractQueryString`, () => {
       `)
 
     const query = await collectionExtractQueryString(
-      createPath(`src/pages/{Things.bar}`)
+      createPath(`src/pages/{Things.bar}`),
+      reporter
     )
 
     expect(query).toMatchInlineSnapshot(
       `"{allThings(filter:{name:{nin:[\\"stuff\\"]}}){nodes{bar,id}}}"`
     )
   })
 
-  it(`throws an error if you forget the fragment`, async () => {
+  it(`reports an error if you forget the fragment`, async () => {
     patchReadFileSync(`
       import { unstable_collectionGraphql } from "gatsby"
       export const collectionQuery = unstable_collectionGraphql\`  
         { allThings(filter: { name: { nin: ["stuff"] }}) { nodes { id } } }
       \`
       `)
 
-    expect(() =>
-      collectionExtractQueryString(createPath(`src/pages/{Things.bar}`))
-    ).toThrow()
+    await collectionExtractQueryString(
+      createPath(`src/pages/{Things.bar}`),
+      reporter
+    )
+
+    expect(reporter.error).toBeCalled()
   })
 })
diff --git a/packages/gatsby-plugin-page-creator/src/__tests__/derive-path.ts b/packages/gatsby-plugin-page-creator/src/__tests__/derive-path.ts
@@ -1,31 +1,44 @@
 import { derivePath } from "../derive-path"
+import reporter from "gatsby-cli/lib/reporter"
 
 describe(`derive-path`, () => {
   it(`has basic support`, () => {
-    expect(derivePath(`product/{Product.id}.js`, { id: `1` })).toEqual(
-      `product/1`
-    )
+    expect(
+      derivePath(`product/{Product.id}.js`, { id: `1` }, reporter)
+    ).toEqual(`product/1`)
   })
 
   it(`has nested value support`, () => {
     expect(
-      derivePath(`product/{Product.field__id}.js`, { field: { id: `1` } })
+      derivePath(
+        `product/{Product.field__id}.js`,
+        { field: { id: `1` } },
+        reporter
+      )
     ).toEqual(`product/1`)
   })
 
   it(`has union support`, () => {
     expect(
-      derivePath(`product/{Product.field__(File)__id}.js`, {
-        field: { id: `1` },
-      })
+      derivePath(
+        `product/{Product.field__(File)__id}.js`,
+        {
+          field: { id: `1` },
+        },
+        reporter
+      )
     ).toEqual(`product/1`)
   })
 
   it(`doesnt remove '/' from slug`, () => {
     expect(
-      derivePath(`product/{Product.slug}.js`, {
-        slug: `bar/baz`,
-      })
+      derivePath(
+        `product/{Product.slug}.js`,
+        {
+          slug: `bar/baz`,
+        },
+        reporter
+      )
     ).toEqual(`product/bar/baz`)
   })
 })
diff --git a/packages/gatsby-plugin-page-creator/src/__tests__/is-valid-collection-path-implementation.ts b/packages/gatsby-plugin-page-creator/src/__tests__/is-valid-collection-path-implementation.ts
@@ -31,7 +31,7 @@ describe(`isValidCollectionPathImplementation`, () => {
 
     isValidCollectionPathImplementation(compatiblePath(path), reporter)
     expect(reporter.panicOnBuild)
-      .toBeCalledWith(`Collection page builder encountered an error parsing the filepath. To use collection paths the schema to follow is {Model.field}. The problematic part is: ${part}.
+      .toBeCalledWith(`PageCreator: Collection page builder encountered an error parsing the filepath. To use collection paths the schema to follow is {Model.field}. The problematic part is: ${part}.
 filePath: ${compatiblePath(path)}`)
   })
 })
diff --git a/packages/gatsby-plugin-page-creator/src/__tests__/validate-path-query.ts b/packages/gatsby-plugin-page-creator/src/__tests__/validate-path-query.ts
@@ -7,7 +7,7 @@ describe(`validatePathQuery`, () => {
     expect(() => {
       validatePathQuery(`foo/{bar}`, [])
     }).toThrowErrorMatchingInlineSnapshot(`
-"To query node \\"path\\" the \\"filePath\\" argument must be an absolute path, starting with a /
+"PageCreator: To query node \\"path\\" the \\"filePath\\" argument must be an absolute path, starting with a /
 Please change this to: \\"/foo/{bar}\\""
 `)
   })
@@ -16,7 +16,7 @@ Please change this to: \\"/foo/{bar}\\""
     expect(() => {
       validatePathQuery(`/foo/{bar}.js`, [])
     }).toThrowErrorMatchingInlineSnapshot(`
-"To query node \\"path\\" the \\"filePath\\" argument must omit the file extension
+"PageCreator: To query node \\"path\\" the \\"filePath\\" argument must omit the file extension
 Please change /foo/{bar}.js to \\"/foo/{bar}\\""
 `)
   })
@@ -25,7 +25,7 @@ Please change /foo/{bar}.js to \\"/foo/{bar}\\""
     expect(() => {
       validatePathQuery(`/src/pages/foo/{bar}`, [])
     }).toThrowErrorMatchingInlineSnapshot(`
-"To query node \\"path\\" the \\"filePath\\" argument must omit the src/pages prefix.
+"PageCreator: To query node \\"path\\" the \\"filePath\\" argument must omit the src/pages prefix.
 Please change this to: \\"foo/{bar}\\""
 `)
   })
@@ -34,7 +34,7 @@ Please change this to: \\"foo/{bar}\\""
     expect(() => {
       validatePathQuery(`/foo/{bar}/index`, [])
     }).toThrowErrorMatchingInlineSnapshot(`
-"To query node \\"path\\" the \\"filePath\\" argument must omit index.
+"PageCreator: To query node \\"path\\" the \\"filePath\\" argument must omit index.
 Please change this to: \\"/foo/{bar}/\\""
 `)
   })
@@ -43,7 +43,7 @@ Please change this to: \\"/foo/{bar}/\\""
     expect(() => {
       validatePathQuery(`/foo/{bar}`, [`.js`, `.ts`, `.mjs`])
     }).toThrowErrorMatchingInlineSnapshot(`
-"To query node \\"path\\" the \\"filePath\\" argument must represent a file that exists.
+"PageCreator: To query node \\"path\\" the \\"filePath\\" argument must represent a file that exists.
 Unable to find a file at: \\"<PROJECT_ROOT>/src/pages/foo/{bar}\\""
 `)
   })

diff --git a/packages/gatsby-plugin-page-creator/src/collection-extract-query-string.ts b/packages/gatsby-plugin-page-creator/src/collection-extract-query-string.ts
@@ -4,14 +4,16 @@ import { getGraphQLTag } from "babel-plugin-remove-graphql-queries"
 import fs from "fs-extra"
 import traverse from "@babel/traverse"
 import { extractModel } from "./path-utils"
+import { Reporter } from "gatsby"
 
 // This Function opens up the actual collection file and extracts the queryString used in the
 export function collectionExtractQueryString(
-  absolutePath: string
+  absolutePath: string,
+  reporter: Reporter
 ): string | null {
   let queryString: string | null = null
 
-  const modelType = extractModel(absolutePath)
+  let modelType = extractModel(absolutePath)
 
   // This can happen if you have an invalid path and you are trying to query for that path
   // our path graphql resolution logic does not validate the path before calling this
@@ -37,9 +39,16 @@ export function collectionExtractQueryString(
             if (!text) return
 
             if (text.includes(`...CollectionPagesQueryFragment`) === false) {
-              throw new Error(
-                `Your collection graphql query is incorrect. You must use the fragment "...CollectionPagesQueryFragment" to pull data nodes`
+              reporter.error(
+                `PageCreator: Your collection graphql query is incorrect. You must use the fragment "...CollectionPagesQueryFragment" to pull data nodes
+
+Please fix the query from ${absolutePath.split(`src/pages`)[1]}.
+
+Offending query: ${text}`
               )
+              queryString = ``
+              modelType = ``
+              return
             }
 
             queryString = text

diff --git a/packages/gatsby-plugin-page-creator/src/create-page-wrapper.ts b/packages/gatsby-plugin-page-creator/src/create-page-wrapper.ts
@@ -39,7 +39,7 @@ export function createPage(
   if (pathIsCollectionBuilder(absolutePath)) {
     trackFeatureIsUsed(`UnifiedRoutes:collection-page-builder`)
     if (!process.env.GATSBY_EXPERIMENTAL_ROUTING_APIS) {
-      throw new Error(
+      reporter.panic(
         `PageCreator: Found a collection route, but the proper env was not set to enable this experimental feature. Please run again with \`GATSBY_EXPERIMENTAL_ROUTING_APIS=1\` to enable.`
       )
     }
@@ -57,7 +57,7 @@ export function createPage(
   if (pathIsClientOnlyRoute(absolutePath)) {
     trackFeatureIsUsed(`UnifiedRoutes:client-page-builder`)
     if (!process.env.GATSBY_EXPERIMENTAL_ROUTING_APIS) {
-      throw new Error(`PageCreator: Found a client route, but the proper env was not set to enable this experimental feature. Please run again with \`GATSBY_EXPERIMENTAL_ROUTING_APIS=1\` to enable.
+      reporter.panic(`PageCreator: Found a client route, but the proper env was not set to enable this experimental feature. Please run again with \`GATSBY_EXPERIMENTAL_ROUTING_APIS=1\` to enable.
 Skipping creating pages for ${absolutePath}`)
     }
     createClientOnlyPage(filePath, absolutePath, actions)

diff --git a/packages/gatsby-plugin-page-creator/src/create-pages-from-collection-builder.ts b/packages/gatsby-plugin-page-creator/src/create-pages-from-collection-builder.ts
@@ -19,7 +19,7 @@ export async function createPagesFromCollectionBuilder(
   reporter: Reporter
 ): Promise<void> {
   if (isValidCollectionPathImplementation(absolutePath, reporter) === false) {
-    watchCollectionBuilder(absolutePath, ``, [], actions, () =>
+    watchCollectionBuilder(absolutePath, ``, [], actions, reporter, () =>
       createPagesFromCollectionBuilder(
         filePath,
         absolutePath,
@@ -32,11 +32,11 @@ export async function createPagesFromCollectionBuilder(
   }
 
   // 1. Query for the data for the collection to generate pages
-  const queryString = collectionExtractQueryString(absolutePath)
+  const queryString = collectionExtractQueryString(absolutePath, reporter)
 
   // 1.a  If the query string is not findable, we can't move on. So we stop and watch
   if (queryString === null) {
-    watchCollectionBuilder(absolutePath, ``, [], actions, () =>
+    watchCollectionBuilder(absolutePath, ``, [], actions, reporter, () =>
       createPagesFromCollectionBuilder(
         filePath,
         absolutePath,
@@ -55,22 +55,28 @@ export async function createPagesFromCollectionBuilder(
   // 1.a If it fails, we need to inform the user and exit early
   if (!data || errors) {
     reporter.error(
-      `Tried to create pages from the collection builder.
+      `PageCreator: Tried to create pages from the collection builder.
 Unfortunately, the query came back empty. There may be an error in your query.
 
 file: ${absolutePath}
 
 ${errors.map(error => error.message).join(`\n`)}`.trim()
     )
 
-    watchCollectionBuilder(absolutePath, queryString, [], actions, () =>
-      createPagesFromCollectionBuilder(
-        filePath,
-        absolutePath,
-        actions,
-        graphql,
-        reporter
-      )
+    watchCollectionBuilder(
+      absolutePath,
+      queryString,
+      [],
+      actions,
+      reporter,
+      () =>
+        createPagesFromCollectionBuilder(
+          filePath,
+          absolutePath,
+          actions,
+          graphql,
+          reporter
+        )
     )
 
     return
@@ -84,7 +90,7 @@ ${errors.map(error => error.message).join(`\n`)}`.trim()
 
   if (nodes) {
     reporter.info(
-      `   Creating ${nodes.length} page${
+      `   PageCreator: Creating ${nodes.length} page${
         nodes.length > 1 ? `s` : ``
       } from ${filePath}`
     )
@@ -94,7 +100,7 @@ ${errors.map(error => error.message).join(`\n`)}`.trim()
   //    the watcher will use this data to delete the pages if the query changes significantly.
   const paths = nodes.map((node: Record<string, object>) => {
     // URL path for the component and node
-    const path = createPath(derivePath(filePath, node))
+    const path = createPath(derivePath(filePath, node, reporter))
     // Params is supplied to the FE component on props.params
     const params = getCollectionRouteParams(createPath(filePath), path)
     // nodeParams is fed to the graphql query for the component
@@ -115,13 +121,19 @@ ${errors.map(error => error.message).join(`\n`)}`.trim()
     return path
   })
 
-  watchCollectionBuilder(absolutePath, queryString, paths, actions, () =>
-    createPagesFromCollectionBuilder(
-      filePath,
-      absolutePath,
-      actions,
-      graphql,
-      reporter
-    )
+  watchCollectionBuilder(
+    absolutePath,
+    queryString,
+    paths,
+    actions,
+    reporter,
+    () =>
+      createPagesFromCollectionBuilder(
+        filePath,
+        absolutePath,
+        actions,
+        graphql,
+        reporter
+      )
   )
 }
diff --git a/packages/gatsby-plugin-page-creator/src/derive-path.ts b/packages/gatsby-plugin-page-creator/src/derive-path.ts
@@ -7,12 +7,17 @@ import {
   extractAllCollectionSegments,
   switchToPeriodDelimiters,
 } from "./path-utils"
+import { Reporter } from "gatsby"
 
 // Generates the path for the page from the file path
 // product/{Product.id}.js => /product/:id, pulls from nodes.id
 // product/{Product.sku__en} => product/:sku__en pulls from nodes.sku.en
 // blog/{MarkdownRemark.parent__(File)__relativePath}} => blog/:slug pulls from nodes.parent.relativePath
-export function derivePath(path: string, node: Record<string, any>): string {
+export function derivePath(
+  path: string,
+  node: Record<string, any>,
+  reporter: Reporter
+): string {
   // 1.  Remove the extension
   let pathWithoutExtension = removeFileExtension(path)
 
@@ -33,10 +38,10 @@ export function derivePath(path: string, node: Record<string, any>): string {
 
     // 3.c  log error if the key does not exist on node
     if (nodeValue === undefined) {
-      console.error(
-        `CollectionBuilderError: Could not find value in the following node for key ${slugPart} (transformed to ${key})`
+      reporter.error(
+        `PageCreator: Could not find value in the following node for key ${slugPart} (transformed to ${key})`
       )
-      console.log(JSON.stringify(node, null, 4))
+      reporter.log(JSON.stringify(node, null, 4))
       return
     }