Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Report GraphQL system errors as expected #4811

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Report GraphQL system errors as expected #4811

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions packages/cli-kit/src/private/node/api/graphql.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import {GraphQLClientError, sanitizedHeadersOutput} from './headers.js'
import {sanitizeURL} from './urls.js'
import {stringifyMessage, outputContent, outputToken, outputDebug} from '../../../public/node/output.js'
import {AbortError} from '../../../public/node/error.js'
import {ClientError, Variables} from 'graphql-request'

export function debugLogRequestInfo(
Expand Down Expand Up @@ -43,12 +42,7 @@ ${outputToken.json(error.response.errors)}
Request ID: ${requestId}
`
}
let mappedError: Error
if (status < 500) {
mappedError = new GraphQLClientError(errorMessage, status, error.response.errors)
} else {
mappedError = new AbortError(errorMessage)
}
const mappedError: Error = new GraphQLClientError(errorMessage, status, error.response.errors)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not strictly necessary, but using the same type here allows us to retain information that is otherwise lost. This includes the specific error messages, the status code, and stack information.

Not sure of the original intent of "Client" here. Is it meant to convey a 4xx class error, or an error received by the GraphQL client? If the former, then this new usage would require a rename, at least as currently written,.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its the latter but this is one of the older corners of things, I don't think there's any issue with making larger changes here if you see fit

I don't think we need to say mappedError: Error if its only going to ever be a GraphQLClientError now.

mappedError.stack = error.stack
return mappedError
} else {
Expand Down
33 changes: 32 additions & 1 deletion packages/cli-kit/src/public/node/error.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ export async function handler(error: unknown): Promise<unknown> {
} else if (typeof error === 'string') {
fatal = new BugError(error)
} else if (error instanceof Error) {
fatal = new BugError(error.message)
fatal = isGraphQLError(error) ? new AbortError(error.message) : new BugError(error.message)
fatal.stack = error.stack
} else {
// errors can come in all shapes and sizes...
Expand Down Expand Up @@ -188,6 +188,9 @@ function isFatal(error: unknown): error is FatalError {
*/
export function shouldReportErrorAsUnexpected(error: unknown): boolean {
if (!isFatal(error)) {
if (isInternalGraphQLError(error)) {
return false
}
// this means its not one of the CLI wrapped errors
if (error instanceof Error) {
const message = error.message
Expand Down Expand Up @@ -234,3 +237,31 @@ function errorMessageImpliesEnvironmentIssue(message: string): boolean {
const anyMatches = environmentIssueMessages.some((issueMessage) => message.includes(issueMessage))
return anyMatches
}

// eslint-disable-next-line @typescript-eslint/no-explicit-any
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like using any here and would appreciate any help in removing it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unknown would be better and reflects what this gets called with better I think. If you combine it with the type narrowing below I think it'll work nicely -- you check at runtime that this is a client error and then you do your extra logic around the nature of the error

function isInternalGraphQLError(error: any): boolean {
if (!isGraphQLError(error)) {
return false
}

const extensions = error?.errors?.[0]?.extensions
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and below, we are looking at only the first error, so this still needs work. The logic shoujld be that the error is internal if any of the errors in the array are internal.


// App Management errors have a category that can be used to determine if the error is internal
const category: string | undefined = extensions?.app_errors?.errors?.[0]?.category
if (category) {
return category === 'internal'
}

// Partners errors have a code that can be used to determine if the error is internal
const code: string | undefined = extensions?.code
if (code) {
return parseInt(code, 10) >= 500
}

return true
}

// eslint-disable-next-line @typescript-eslint/no-explicit-any
function isGraphQLError(error: any): boolean {
return error?.name === 'GraphQLClientError'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My attempts to use something like instanceof did not work. This another place where I could use some help with typing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what you want here is a narrowing function. This is something that takes some input at runtime and if true, makes some implication about the provided value https://www.typescriptlang.org/docs/handbook/2/narrowing.html#using-type-predicates

So in this case, maybe roughly:

function isGraphQLError(error: unknown): error is GraphQLClientError {
  return error !== null && typeof error === 'object' && 'name' in error && error.name === 'GraphQLClientError'
}

then elsewhere you can do

if (isGraphQLError(error)) {
  // safe to treat this as a GraphQLClientError
  error.someGraphQLThing.whatever()
}

}
Loading