feat(gatsby,gatsby-cli): Pass an errorMap to reporter.error #27176

abhiaiyer91 · 2020-09-29T17:57:59Z

This PR has enhancements to the reporter (in gatsby cli) and "local reporter" (in gatsby).

The Reporter Class has added
errorMap -> Plugins can register new structured errors in onPreInit by calling reporter.setErrorMap

gatsby-cloud · 2020-09-29T18:23:15Z

Gatsby Cloud Build Report

gatsby

🎉 Your build was successful! See the Deploy preview here.

Build Details

View the build logs here.

🕐 Build time: 20m

…rMap

wardpeet

This looks great! I'm mostly wondering about if we want to make the reporter public API broader.

wardpeet · 2020-10-02T13:03:10Z

integration-tests/structured-logging/__tests__/plugin-errors.js

+    expect(events).toEqual(
+      expect.arrayContaining([
+        expect.objectContaining({
+          type: `LOG_ACTION`,
+          action: expect.objectContaining({
+            type: `LOG`,
+            payload: expect.objectContaining({
+              text: "setErrorMap",
+              level: "INFO",
+            }),
+          }),
+        }),
+      ])
+    )


Should we have this in our test? if we want to test if setErrorMap is tested we should use jest.spyOn on the reporter instead. I'm not sure we have to test it cause the below code will validate that the custom error is set.

wardpeet · 2020-10-02T13:04:11Z

integration-tests/structured-logging/plugins/structured-plugin-errors/gatsby-node.js

+  reporter.info("setErrorMap")
+
+  if (process.env.PANIC_IN_PLUGIN) {
+    reporter.panic({ id: "TEST_ERROR", context: { someProp: `PANIC!` } })


can we move to a number based one as that's more or less the convention we have. It's probably something we need to force.

Suggested change

reporter.panic({ id: "TEST_ERROR", context: { someProp: `PANIC!` } })

reporter.panic({ id: "666", context: { someProp: `PANIC!` } })

wardpeet · 2020-10-02T13:12:26Z

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

-          originalEnd.apply(activity)
-          runningActivities.delete(activity)
-        }
+    const localReporter = getLocalReporter({ activity, reporter })


Let's move everything inside "extendLocalReporterToCatchPluginErrors"

Suggested change

const localReporter = getLocalReporter({ activity, reporter })

wardpeet · 2020-10-02T13:13:04Z

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

+    const extendedLocalReporter = extendLocalReporterToCatchPluginErrors({
+      reporter: localReporter,
+      pluginName: plugin.name,
+      runningActivities,
+    })


Suggested change

const extendedLocalReporter = extendLocalReporterToCatchPluginErrors({

reporter: localReporter,

pluginName: plugin.name,

runningActivities,

})

const extendedLocalReporter = extendReporterWithCatchPluginErrorAndErrorMap({

reporter: localReporter,

pluginName: plugin.name,

activity,

runningActivities,

})

wardpeet · 2020-10-02T18:33:34Z

packages/gatsby-cli/src/reporter/reporter.ts

+  setErrorMapWithPluginName = (name: string) => (
+    entry: Record<ErrorId, IErrorMapEntry>
+  ): void => {
+    const entries = Object.entries(entry)
+
+    const newErrorMap = entries.reduce((memo, [key, val]) => {
+      memo[`${name}_${key}`] = val
+
+      return memo
+    }, {})
+
+    this.setErrorMap(newErrorMap)
+  }


do we want to expose it on the reporter? Maybe it's better to export a utility that we use in api-runner. It feels like a private function

packages/gatsby-cli/src/reporter/reporter.ts

wardpeet · 2020-10-02T18:45:00Z

packages/gatsby-cli/src/reporter/reporter.ts

+  logFailureWithPluginName = ({
+    methodName,
+    pluginName,
+  }: {
+    methodName: string
+    pluginName: string
+  }) => (errorMeta: ErrorMeta, error?: Error | Array<Error>): void => {
+    if (typeof errorMeta === `object`) {
+      const id = errorMeta && errorMeta[`id`]
+
+      if (id) {
+        errorMeta[`id`] = `${pluginName}_${id}`
+      }
+    }
+
+    this[methodName](errorMeta, error)
+  }


Same her do we want it on the reporter and not as a utility? I want to limit the public api from reporter.

utility is good

wardpeet

Added some nitpicks to make code side-effect free

wardpeet · 2020-10-04T14:20:38Z

packages/gatsby-cli/src/structured-errors/__tests__/construct-error.ts

+test(`it constructs an error from the supplied errorMap`, () => {
+  const error = constructError(
+    { details: { id: `1337`, context: { someProp: `Error!` } } },
+    {
+      "1337": {
+        text: (context): string => `Error text is ${context.someProp} `,
+        level: Level.ERROR,
+        docsUrl: `https://www.gatsbyjs.org/docs/gatsby-cli/#new`,
+      },
+    }
+  )
+
+  expect(error.code).toBe(`1337`)
+  expect(error.docsUrl).toBe(`https://www.gatsbyjs.org/docs/gatsby-cli/#new`)
+})


Can you add a test so we check if original errorMap cannot be overridden.

wardpeet · 2020-10-04T14:21:52Z

packages/gatsby-cli/src/structured-errors/construct-error.ts

+  if (id) {
+    // Look at original errorMap, ids cannot be overwritten
+    if (errorMap[id]) {
+      errorMapEntry = errorMap[id]
+    }
+
+    if (suppliedErrorMap[id]) {
+      errorMapEntry = suppliedErrorMap[id]
+    }
+  }


It's probably my mistake in a previous comment but it should be

Suggested change

if (id) {

// Look at original errorMap, ids cannot be overwritten

if (errorMap[id]) {

errorMapEntry = errorMap[id]

}

if (suppliedErrorMap[id]) {

errorMapEntry = suppliedErrorMap[id]

}

}

if (id) {

// Look at original errorMap, ids cannot be overwritten

if (errorMap[id]) {

errorMapEntry = errorMap[id]

} else if (suppliedErrorMap[id]) {

errorMapEntry = suppliedErrorMap[id]

}

}

wardpeet · 2020-10-04T14:24:20Z

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

+function extendErrorIdWithPluginName(pluginName, errorMeta) {
+  if (typeof errorMeta === `object`) {
+    const id = errorMeta && errorMeta[`id`]
+
+    if (id) {
+      errorMeta[`id`] = `${pluginName}_${id}`
+    }
+  }
+}


Let's keep this side-effect free so we don't augment the object but instead create a new one.

Suggested change

function extendErrorIdWithPluginName(pluginName, errorMeta) {

if (typeof errorMeta === `object`) {

const id = errorMeta && errorMeta[`id`]

if (id) {

errorMeta[`id`] = `${pluginName}_${id}`

}

}

}

function extendErrorIdWithPluginName(pluginName, errorMeta) {

if (typeof errorMeta === `object`) {

const id = errorMeta && errorMeta[`id`]

if (id) {

return {

...errorMeta,

id: `${pluginName}_${id}`

}

}

}

return errorMeta;

}

wardpeet · 2020-10-04T14:25:28Z

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

+    error = (errorMeta, error) => {
+      extendErrorIdWithPluginName(pluginName, errorMeta)
+
+      return reporter.error(errorMeta, error)
+    }
+
+    panic = (errorMeta, error) => {
+      extendErrorIdWithPluginName(pluginName, errorMeta)
+
+      return reporter.panic(errorMeta, error)
+    }
+
+    panicOnBuild = (errorMeta, error) => {
+      extendErrorIdWithPluginName(pluginName, errorMeta)
+
+      return reporter.panicOnBuild(errorMeta, error)
+    }


With the above change we should be able to move to

Suggested change

error = (errorMeta, error) => {

extendErrorIdWithPluginName(pluginName, errorMeta)

return reporter.error(errorMeta, error)

}

panic = (errorMeta, error) => {

extendErrorIdWithPluginName(pluginName, errorMeta)

return reporter.panic(errorMeta, error)

}

panicOnBuild = (errorMeta, error) => {

extendErrorIdWithPluginName(pluginName, errorMeta)

return reporter.panicOnBuild(errorMeta, error)

}

error = (errorMeta, error) => {

return reporter.error(extendErrorIdWithPluginName(pluginName, errorMeta), error)

}

panic = (errorMeta, error) => {

return reporter.panic(extendErrorIdWithPluginName(pluginName, errorMeta), error)

}

panicOnBuild = (errorMeta, error) => {

return reporter.panicOnBuild(extendErrorIdWithPluginName(pluginName, errorMeta), error)

}

wardpeet · 2020-10-04T14:26:58Z

packages/gatsby-cli/src/reporter/reporter.ts

+  getErrorMap = (): Record<ErrorId, IErrorMapEntry> =>
+    // TODO: We always spread the internal error map to ensure our keys do not get overwritten
+    this.errorMap


Let's not expose this function, so users can't augment it themselves.

Suggested change

getErrorMap = (): Record<ErrorId, IErrorMapEntry> =>

// TODO: We always spread the internal error map to ensure our keys do not get overwritten

this.errorMap

wardpeet · 2020-10-04T14:27:15Z

packages/gatsby-cli/src/reporter/reporter.ts

@@ -136,7 +158,8 @@ class Reporter {
      }
    }

-    const structuredError = constructError({ details })
+    const structuredError = constructError({ details }, this.getErrorMap())


Suggested change

const structuredError = constructError({ details }, this.getErrorMap())

const structuredError = constructError({ details }, this.errorMap)

wardpeet · 2020-10-05T09:40:51Z

packages/gatsby-cli/src/structured-errors/error-schema.ts

@@ -20,6 +20,7 @@ export const errorSchema: Joi.ObjectSchema<IStructuredError> = Joi.object().keys
        })
      )
      .allow(null),
+    errorCategory: Joi.string().valid([`USER`, `SYSTEM`]),


it's already part of an error, should we prefix it with error?

Suggested change

errorCategory: Joi.string().valid([`USER`, `SYSTEM`]),

category: Joi.string().valid([`USER`, `SYSTEM`]),

:

LekoArts · 2020-10-05T12:52:26Z

Codewise this LGTM overall (Ward left some good comments), but got some questions around this (don't know if it was discussed already):

We use 5-digit numbers for the errors so that those are better searchable in Google than e.g. 1 as an ID. Can/Should we enforce the same format for plugin errors?
Since plugins can't override already existing errors from our error map we should at least show a warning somewhere I guess? Otherwise the error reporting silently fails/displays the wrong error message.
Our error map has a loose format, e.g. all GraphQL errors are prefixed with 859 and we're currently at 85927.
- What happens when a plugin developer decides to claim 85928 and we add the same ID sometime later to our own errorMap?
- What happens when two plugins have the same ID?
- Should we enforce certain ranges for our internal errorMap and the external?

julienp · 2020-10-05T12:59:56Z

packages/gatsby-cli/src/structured-errors/error-schema.ts

@@ -20,6 +20,7 @@ export const errorSchema: Joi.ObjectSchema<IStructuredError> = Joi.object().keys
        })
      )
      .allow(null),
+    category: Joi.string().valid([`USER`, `SYSTEM`]),


What would we chose as category for "Wordpress host is down right now and we're getting errors retrieving data" ? Maybe we can have a THIRD_PARTY category for that.

good point, I would have marked that as user

THIRD_PARTY added

abhiaiyer91 · 2020-10-05T18:01:04Z

Codewise this LGTM overall (Ward left some good comments), but got some questions around this (don't know if it was discussed already):

We use 5-digit numbers for the errors so that those are better searchable in Google than e.g. 1 as an ID. Can/Should we enforce the same format for plugin errors?

Since plugins can't override already existing errors from our error map we should at least show a warning somewhere I guess? Otherwise the error reporting silently fails/displays the wrong error message.

Our error map has a loose format, e.g. all GraphQL errors are prefixed with 859 and we're currently at 85927.

What happens when a plugin developer decides to claim 85928 and we add the same ID sometime later to our own errorMap?

What happens when two plugins have the same ID?

Should we enforce certain ranges for our internal errorMap and the external?

Are people actually searching these? And do we have error explainers like react does? If not and if we're not planning on doing so, whats the point of the ids
We can log something

Internal Error map wins
The error maps are prefixed with plugin name so they wont clash like that
We can but to me the current structure doesnt have rhyme or reason, atleast not ones i see clearly

LekoArts · 2020-10-06T07:13:16Z

People are using those IDs for their descriptions in all places and yeah, they also use it for searching for things. We haven't gotten dedicated docs pages for each error but that's something we have as an item on the list for potential next projects
👍
- Okay, as long as we have it documented and optionally log something it should be discover-able enough
- 👍
- While the numbers were certainly chosen randomly in the start, the system behind it is something like: For a dedicated area of errors choose the same 4-digits and then count up. So e.g. HTML/SSR errors are 9531x, Webpack errors are 9812x etc. If we would split up the error 98123 into more errors it would be 98125, 98126 etc.

So overall it's quite arbitrarily chosen and I don't have a strong opinion on the Should we enforce certain ranges for our internal errorMap and the external? question, just an idea to minimize the clashing of internal <-> external errorMap :)

wardpeet · 2020-10-07T15:26:33Z

packages/gatsby-cli/src/structured-errors/error-schema.ts

@@ -20,6 +20,7 @@ export const errorSchema: Joi.ObjectSchema<IStructuredError> = Joi.object().keys
        })
      )
      .allow(null),
+    category: Joi.string().valid([`USER`, `SYSTEM`]),


Suggested change

category: Joi.string().valid([`USER`, `SYSTEM`]),

category: Joi.string().valid([`USER`, `SYSTEM`, `THIRD_PARTY`]),

wardpeet

LGTM

wesrice · 2020-10-08T13:47:54Z

👋 Hey. This doesn't play well with Gatsby + TypeScript. I'll open a new issue once I understand the problem better.

3:31 Cannot find module 'gatsby-cli/src/structured-errors/error-map' or its corresponding type declarations.
    1 | import { Actions, ActivityStatuses, ActivityTypes } from "../constants";
    2 | import { IStructuredError } from "../../structured-errors/types";
  > 3 | import { ErrorCategory } from "gatsby-cli/src/structured-errors/error-map";
      |                               ^
    4 | export interface IGatsbyCLIState {
    5 |     messages: Array<ILog>;
    6 |     activities: {

abhiaiyer91 · 2020-10-08T14:12:12Z

@wesrice ahhh auto import in vscode. my bad, i have this fixed in this PR #27340

…#27176)

abhiaiyer91 added 2 commits September 29, 2020 10:56

Pass an error map to the reporter.error

5c2263c

Pass an error map to the reporter.error

fb83545

gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Sep 29, 2020

abhiaiyer91 marked this pull request as draft September 29, 2020 17:58

abhiaiyer91 changed the title ~~~~ Pass an errorMap to reporter.error Sep 29, 2020

LekoArts added topic: cli Related to the Gatsby CLI and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Sep 30, 2020

abhiaiyer91 added 3 commits September 30, 2020 14:09

Switch up approach to error map

e0e2704

Switch up approach to error map

7007663

remove extraneous change

a062d3a

abhiaiyer91 requested review from wardpeet, sidharthachatterjee and julienp September 30, 2020 21:18

abhiaiyer91 added 8 commits October 1, 2020 08:30

Extend reporter to add plugin name to error map

1749785

Extend reporter to add plugin name to error map

875a0fd

Extend reporter to add plugin name to error map

09b8999

Extend reporter to add plugin name to error map

9b8d8ec

Integration test for plugin errors

000104f

Integration test for plugin errors

54e9f7a

Add test for wrapper function

a2fa61b

Add test for wrapper function

c495188

abhiaiyer91 marked this pull request as ready for review October 2, 2020 12:55

Merge branch 'master' of github.com:gatsbyjs/gatsby into registerErro…

dd2b586

…rMap

wardpeet reviewed Oct 2, 2020

View reviewed changes

abhiaiyer91 added 4 commits October 2, 2020 11:58

Code review updates

3dd95c4

Code review updates

1785402

Code review updates

d91fcf9

Code review updates

1904bf2

abhiaiyer91 mentioned this pull request Oct 4, 2020

feat(gatsby, gatsby-cli, gatsby-source-contentful): contentful structured errors #27277

Closed

wardpeet reviewed Oct 4, 2020

View reviewed changes

abhiaiyer91 added 3 commits October 4, 2020 07:34

PR review comments

ba430f9

PR review comments

66f8f9c

PR review comments

ac133c1

wardpeet changed the title ~~Pass an errorMap to reporter.error~~ feat(gatsby,gatsby-cli): Pass an errorMap to reporter.error Oct 4, 2020

Pipe errorCategory where it needs to

d8f2d46

wardpeet reviewed Oct 5, 2020

View reviewed changes

change name to category

8a87264

:

julienp reviewed Oct 5, 2020

View reviewed changes

add third party category

3ddd990

wardpeet reviewed Oct 7, 2020

View reviewed changes

Third party in joi schema

e1de003

wardpeet approved these changes Oct 7, 2020

View reviewed changes

wardpeet merged commit 56402db into master Oct 7, 2020

wardpeet deleted the registerErrorMap branch October 7, 2020 17:05

wardpeet added the hacktoberfest-accepted label Oct 8, 2020

This was referenced Oct 8, 2020

gatsby-cli 2.12.105: Cannot find module 'gatsby-cli/src/structured-errors/error-map' or its corresponding type declarations. #27339

Closed

Add structured reporting with codes to fetch-graphql gatsbyjs/gatsby-source-wordpress-experimental#213

Closed

mehov mentioned this pull request Jan 12, 2022

Need clarification on disabling PARALLEL_QUERY_RUNNING at a runtime #34467

Closed

2 tasks

pragmaticpat pushed a commit to pragmaticpat/gatsby that referenced this pull request Apr 28, 2022

feat(gatsby,gatsby-cli): Pass an errorMap to reporter.error (gatsbyjs…

33dda51

…#27176)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(gatsby,gatsby-cli): Pass an errorMap to reporter.error #27176

feat(gatsby,gatsby-cli): Pass an errorMap to reporter.error #27176

abhiaiyer91 commented Sep 29, 2020 •

edited

gatsby-cloud bot commented Sep 29, 2020 •

edited

wardpeet left a comment

wardpeet Oct 2, 2020

wardpeet Oct 2, 2020

wardpeet Oct 2, 2020

wardpeet Oct 2, 2020

wardpeet Oct 2, 2020

wardpeet Oct 2, 2020

abhiaiyer91 Oct 2, 2020

wardpeet left a comment

wardpeet Oct 4, 2020

wardpeet Oct 4, 2020

wardpeet Oct 4, 2020

wardpeet Oct 4, 2020

wardpeet Oct 4, 2020

wardpeet Oct 4, 2020

wardpeet Oct 5, 2020

LekoArts commented Oct 5, 2020 •

edited

julienp Oct 5, 2020

wardpeet Oct 5, 2020

abhiaiyer91 Oct 5, 2020

abhiaiyer91 commented Oct 5, 2020

LekoArts commented Oct 6, 2020

wardpeet Oct 7, 2020

wardpeet left a comment

wesrice commented Oct 8, 2020

abhiaiyer91 commented Oct 8, 2020

	reporter.panic({ id: "TEST_ERROR", context: { someProp: `PANIC!` } })
	reporter.panic({ id: "666", context: { someProp: `PANIC!` } })

	getErrorMap = (): Record<ErrorId, IErrorMapEntry> =>
	// TODO: We always spread the internal error map to ensure our keys do not get overwritten
	this.errorMap

	const structuredError = constructError({ details }, this.getErrorMap())
	const structuredError = constructError({ details }, this.errorMap)

	errorCategory: Joi.string().valid([`USER`, `SYSTEM`]),
	category: Joi.string().valid([`USER`, `SYSTEM`]),

feat(gatsby,gatsby-cli): Pass an errorMap to reporter.error #27176

feat(gatsby,gatsby-cli): Pass an errorMap to reporter.error #27176

Conversation

abhiaiyer91 commented Sep 29, 2020 • edited

gatsby-cloud bot commented Sep 29, 2020 • edited

Gatsby Cloud Build Report

Build Details

wardpeet 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

wardpeet 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

LekoArts commented Oct 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhiaiyer91 commented Oct 5, 2020

LekoArts commented Oct 6, 2020

Choose a reason for hiding this comment

wardpeet left a comment

Choose a reason for hiding this comment

wesrice commented Oct 8, 2020

abhiaiyer91 commented Oct 8, 2020

abhiaiyer91 commented Sep 29, 2020 •

edited

gatsby-cloud bot commented Sep 29, 2020 •

edited

LekoArts commented Oct 5, 2020 •

edited