Merge pull request #4724 from Shopify/backport-bug-logging

Backport bug logging

packages/cli-kit/src/public/node/api/admin.test.ts

@@ -56,7 +56,7 @@ describe('admin-graphql-api', () => {
     expect(graphqlRequest).toHaveBeenLastCalledWith({
       query: 'query',
       api: 'Admin',
-      url: 'https://store/admin/api/2022-01/graphql.json',
+      url: 'https://store.myshopify.com/admin/api/2022-01/graphql.json',
       token,
       variables: {variables: 'variables'},
     })

packages/cli-kit/src/public/node/api/admin.ts

@@ -5,6 +5,7 @@ import {BugError, AbortError} from '../error.js'
 import {restRequestBody, restRequestHeaders, restRequestUrl} from '../../../private/node/api/rest.js'
 import {fetch} from '../http.js'
 import {PublicApiVersions} from '../../../cli/api/graphql/admin/generated/public_api_versions.js'
+import {normalizeStoreFqdn} from '../context/fqdn.js'
 import {ClientError, Variables} from 'graphql-request'
 import {TypedDocumentNode} from '@graphql-typed-document-node/core'
 
@@ -19,7 +20,8 @@ import {TypedDocumentNode} from '@graphql-typed-document-node/core'
 export async function adminRequest<T>(query: string, session: AdminSession, variables?: GraphQLVariables): Promise<T> {
   const api = 'Admin'
   const version = await fetchLatestSupportedApiVersion(session)
-  const url = adminUrl(session.storeFqdn, version)
+  const store = await normalizeStoreFqdn(session.storeFqdn)
+  const url = adminUrl(store, version)
   return graphqlRequest({query, api, url, token: session.token, variables})
 }
 
@@ -44,8 +46,9 @@ export async function adminRequestDoc<TResult, TVariables extends Variables>(
   if (!version) {
     apiVersion = await fetchLatestSupportedApiVersion(session)
   }
+  const store = await normalizeStoreFqdn(session.storeFqdn)
   const opts = {
-    url: adminUrl(session.storeFqdn, apiVersion),
+    url: adminUrl(store, apiVersion),
     api: 'Admin',
     token: session.token,
   }
@@ -99,8 +102,17 @@ async function fetchApiVersions(session: AdminSession): Promise<ApiVersion[]> {
         )})`,
         outputContent`If you're not the owner, create a dev store staff account for yourself`,
       )
+    } else if (error instanceof ClientError) {
+      throw new BugError(
+        `Unknown client error connecting to your store ${session.storeFqdn}: ${error.message} ${error.response.status} ${error.response.data}`,
+      )
+    } else {
+      throw new BugError(
+        `Unknown error connecting to your store ${session.storeFqdn}: ${
+          error instanceof Error ? error.message : String(error)
+        }`,
+      )
     }
-    throw new BugError(`Unknown error connecting to your store`)
   }
 }
 
@@ -112,7 +124,7 @@ async function fetchApiVersions(session: AdminSession): Promise<ApiVersion[]> {
  * @returns - Admin API URL.
  */
 export function adminUrl(store: string, version: string | undefined): string {
-  const realVersion = version || 'unstable'
+  const realVersion = version ?? 'unstable'
   return `https://${store}/admin/api/${realVersion}/graphql.json`
 }
 

packages/cli-kit/src/public/node/error-handler.ts

@@ -6,7 +6,7 @@ import {
   AbortSilentError,
   CancelExecution,
   errorMapper,
-  shouldReportError,
+  shouldReportErrorAsUnexpected,
   handler,
   cleanSingleStackTracePath,
 } from './error.js'
@@ -46,7 +46,7 @@ export async function errorHandler(
 const reportError = async (error: unknown, config?: Interfaces.Config): Promise<void> => {
   // categorise the error first
   let exitMode: CommandExitMode = 'expected_error'
-  if (shouldReportError(error)) exitMode = 'unexpected_error'
+  if (shouldReportErrorAsUnexpected(error)) exitMode = 'unexpected_error'
 
   if (config !== undefined) {
     // Log an analytics event when there's an error

packages/cli-kit/src/public/node/error.test.ts

@@ -1,4 +1,4 @@
-import {AbortError, BugError, handler, cleanSingleStackTracePath} from './error.js'
+import {AbortError, BugError, handler, cleanSingleStackTracePath, shouldReportErrorAsUnexpected} from './error.js'
 import {renderFatalError} from './ui.js'
 import {describe, expect, test, vi} from 'vitest'
 
@@ -53,3 +53,21 @@ describe('stack file path helpers', () => {
     expect(cleanSingleStackTracePath(path)).toEqual('/something/there.js')
   })
 })
+
+describe('shouldReportErrorAsUnexpected helper', () => {
+  test('returns true for normal errors', () => {
+    expect(shouldReportErrorAsUnexpected(new Error('test'))).toBe(true)
+  })
+
+  test('returns false for AbortError', () => {
+    expect(shouldReportErrorAsUnexpected(new AbortError('test'))).toBe(false)
+  })
+
+  test('returns true for BugError', () => {
+    expect(shouldReportErrorAsUnexpected(new BugError('test'))).toBe(true)
+  })
+
+  test('returns false for errors that imply environment issues', () => {
+    expect(shouldReportErrorAsUnexpected(new Error('EPERM: operation not permitted, scandir'))).toBe(false)
+  })
+})

packages/cli-kit/src/public/node/error.ts

@@ -181,13 +181,18 @@ function isFatal(error: unknown): error is FatalError {
 }
 
 /**
- * A function that checks if an error should be reported as unhandled.
+ * A function that checks if an error should be reported as unexpected.
  *
  * @param error - Error to be checked.
- * @returns A boolean indicating if the error should be reported.
+ * @returns A boolean indicating if the error should be reported as unexpected.
  */
-export function shouldReportError(error: unknown): boolean {
+export function shouldReportErrorAsUnexpected(error: unknown): boolean {
   if (!isFatal(error)) {
+    // this means its not one of the CLI wrapped errors
+    if (error instanceof Error) {
+      const message = error.message
+      return !errorMessageImpliesEnvironmentIssue(message)
+    }
     return true
   }
   if (error.type === FatalErrorType.Bug) {
@@ -207,3 +212,25 @@ export function cleanSingleStackTracePath(filePath: string): string {
     .replace('file:/', '/')
     .replace(/^\/?[A-Z]:/, '')
 }
+
+/**
+ * There are certain errors that we know are not due to a CLI bug, but are environmental/user error.
+ *
+ * @param message - The error message to check.
+ * @returns A boolean indicating if the error message implies an environment issue.
+ */
+function errorMessageImpliesEnvironmentIssue(message: string): boolean {
+  const environmentIssueMessages = [
+    'EPERM: operation not permitted, scandir',
+    'EACCES: permission denied',
+    'EPERM: operation not permitted, symlink',
+    'This version of npm supports the following node versions',
+    'EBUSY: resource busy or locked, rmdir',
+    'getaddrinfo ENOTFOUND',
+    'Client network socket disconnected before secure TLS connection was established',
+    'spawn EPERM',
+    'socket hang up',
+  ]
+  const anyMatches = environmentIssueMessages.some((issueMessage) => message.includes(issueMessage))
+  return anyMatches
+}