Merge pull request #4599 from Shopify/fd-async-errors

[Theme] Handle async errors

.changeset/polite-suits-remain.md

@@ -0,0 +1,5 @@
+---
+'@shopify/theme': patch
+---
+
+Handle background async errors and avoid process exit in some scenarios.

packages/theme/src/cli/utilities/errors.ts

@@ -0,0 +1,49 @@
+import {AbortError} from '@shopify/cli-kit/node/error'
+import {outputDebug} from '@shopify/cli-kit/node/output'
+import {renderError, renderFatalError} from '@shopify/cli-kit/node/ui'
+
+/**
+ * Renders an error banner for a thrown error with a headline.
+ * @param headline - The headline for the error.
+ * @param error - The error to render.
+ */
+export function renderThrownError(headline: string, error: Error | AbortError) {
+  if (error instanceof AbortError) {
+    error.message = `${headline}\n${error.message}`
+    renderFatalError(error)
+  } else {
+    renderError({headline, body: error.message})
+    outputDebug(`${headline}\n${error.stack ?? error.message}`)
+  }
+}
+
+/**
+ * Creates a function that renders a failed syncing operation without interrupting the process.
+ * @param resource - The file that was being operated on, or a headline for the error.
+ * @param preset - The type of operation that failed.
+ * @returns A function that accepts an error and renders it.
+ */
+export function createSyncingCatchError(resource: string, preset?: 'delete' | 'upload') {
+  const headline =
+    {
+      delete: `Failed to delete file "${resource}" from remote theme.`,
+      upload: `Failed to upload file "${resource}" to remote theme.`,
+      default: resource,
+    }[preset ?? 'default'] ?? resource
+
+  return (error: Error) => {
+    renderThrownError(headline, error)
+  }
+}
+
+/**
+ * Creates a function that renders a failed syncing operation and exits the process.
+ * @param headline - The headline for the error.
+ * @returns A function that accepts an error and renders it.
+ */
+export function createAbortCatchError(headline: string) {
+  return (error: Error): never => {
+    renderThrownError(headline, error)
+    process.exit(1)
+  }
+}

packages/theme/src/cli/utilities/theme-environment/theme-environment.test.ts

@@ -112,6 +112,7 @@ describe('setupDevServer', () => {
     expect(uploadTheme).toHaveBeenCalledWith(developmentTheme, context.session, [], context.localThemeFileSystem, {
       nodelete: true,
       deferPartialWork: true,
+      backgroundWorkCatch: expect.any(Function),
     })
   })
 
@@ -161,6 +162,7 @@ describe('setupDevServer', () => {
     expect(uploadTheme).toHaveBeenCalledWith(developmentTheme, context.session, [], context.localThemeFileSystem, {
       nodelete: true,
       deferPartialWork: true,
+      backgroundWorkCatch: expect.any(Function),
     })
   })
 

packages/theme/src/cli/utilities/theme-environment/theme-environment.ts

@@ -5,6 +5,7 @@ import {getProxyHandler} from './proxy.js'
 import {reconcileAndPollThemeEditorChanges} from './remote-theme-watcher.js'
 import {uploadTheme} from '../theme-uploader.js'
 import {renderTasksToStdErr} from '../theme-ui.js'
+import {createAbortCatchError} from '../errors.js'
 import {createApp, defineEventHandler, defineLazyEventHandler, toNodeListener} from 'h3'
 import {fetchChecksums} from '@shopify/cli-kit/node/themes/api'
 import {createServer} from 'node:http'
@@ -28,22 +29,27 @@ export function setupDevServer(theme: Theme, ctx: DevServerContext) {
 }
 
 function ensureThemeEnvironmentSetup(theme: Theme, ctx: DevServerContext) {
-  const remoteChecksumsPromise = fetchChecksums(theme.id, ctx.session)
-
-  const reconcilePromise = remoteChecksumsPromise.then((remoteChecksums) =>
-    handleThemeEditorSync(theme, ctx, remoteChecksums),
-  )
-
-  const uploadPromise = reconcilePromise.then(async ({updatedRemoteChecksumsPromise}) => {
-    const updatedRemoteChecksums = await updatedRemoteChecksumsPromise
-    return uploadTheme(theme, ctx.session, updatedRemoteChecksums, ctx.localThemeFileSystem, {
-      nodelete: ctx.options.noDelete,
-      deferPartialWork: true,
+  const abort = createAbortCatchError('Failed to perform the initial theme synchronization.')
+
+  const remoteChecksumsPromise = fetchChecksums(theme.id, ctx.session).catch(abort)
+
+  const reconcilePromise = remoteChecksumsPromise
+    .then((remoteChecksums) => handleThemeEditorSync(theme, ctx, remoteChecksums))
+    .catch(abort)
+
+  const uploadPromise = reconcilePromise
+    .then(async ({updatedRemoteChecksumsPromise}) => {
+      const updatedRemoteChecksums = await updatedRemoteChecksumsPromise
+      return uploadTheme(theme, ctx.session, updatedRemoteChecksums, ctx.localThemeFileSystem, {
+        nodelete: ctx.options.noDelete,
+        deferPartialWork: true,
+        backgroundWorkCatch: abort,
+      })
     })
-  })
+    .catch(abort)
 
   return {
-    workPromise: uploadPromise.then((result) => result.workPromise),
+    workPromise: uploadPromise.then((result) => result.workPromise).catch(abort),
     renderProgress: async () => {
       if (ctx.options.themeEditorSync) {
         const {workPromise} = await reconcilePromise

packages/theme/src/cli/utilities/theme-environment/theme-polling.ts

@@ -1,9 +1,11 @@
 import {timestampDateFormat} from '../../constants.js'
+import {renderThrownError} from '../errors.js'
 import {Checksum, Theme, ThemeFileSystem} from '@shopify/cli-kit/node/themes/types'
 import {fetchChecksums, fetchThemeAsset} from '@shopify/cli-kit/node/themes/api'
 import {outputDebug, outputInfo, outputContent, outputToken} from '@shopify/cli-kit/node/output'
 import {AdminSession} from '@shopify/cli-kit/node/session'
-import {renderError} from '@shopify/cli-kit/node/ui'
+import {renderFatalError} from '@shopify/cli-kit/node/ui'
+import {AbortError} from '@shopify/cli-kit/node/error'
 
 const POLLING_INTERVAL = 3000
 class PollingError extends Error {}
@@ -21,19 +23,54 @@ export function pollThemeEditorChanges(
 ) {
   outputDebug('Listening for changes in the theme editor')
 
-  return setTimeout(() => {
-    pollRemoteJsonChanges(targetTheme, session, remoteChecksum, localFileSystem, options)
-      .then((latestChecksums) => {
-        pollThemeEditorChanges(targetTheme, session, latestChecksums, localFileSystem, options)
+  const maxPollingAttempts = 5
+  let failedPollingAttempts = 0
+  let lastError = ''
+  let latestChecksums = remoteChecksum
+
+  const poll = async () => {
+    // Asynchronously wait for the polling interval, similar to a setInterval
+    // but ensure the polling work is done before starting the next interval.
+    await new Promise((resolve) => setTimeout(resolve, POLLING_INTERVAL))
+
+    // eslint-disable-next-line require-atomic-updates
+    latestChecksums = await pollRemoteJsonChanges(targetTheme, session, latestChecksums, localFileSystem, options)
+      .then((checksums) => {
+        failedPollingAttempts = 0
+        lastError = ''
+
+        return checksums
       })
-      .catch((err) => {
-        if (err instanceof PollingError) {
-          renderError({body: err.message})
-        } else {
-          throw err
+      .catch((error: Error) => {
+        failedPollingAttempts++
+
+        if (error.message !== lastError) {
+          lastError = error.message
+          renderThrownError('Error while polling for changes.', error)
+        }
+
+        if (failedPollingAttempts >= maxPollingAttempts) {
+          renderFatalError(
+            new AbortError(
+              'Too many polling errors...',
+              'Please check the errors above and ensure you have a stable internet connection.',
+            ),
+          )
+
+          process.exit(1)
         }
+
+        return latestChecksums
       })
-  }, POLLING_INTERVAL)
+  }
+
+  // eslint-disable-next-line @typescript-eslint/no-misused-promises
+  setTimeout(async () => {
+    while (true) {
+      // eslint-disable-next-line no-await-in-loop
+      await poll()
+    }
+  })
 }
 
 export async function pollRemoteJsonChanges(

packages/theme/src/cli/utilities/theme-fs.test.ts

@@ -16,7 +16,7 @@ import {removeFile, writeFile} from '@shopify/cli-kit/node/fs'
 import {test, describe, expect, vi, beforeEach} from 'vitest'
 import chokidar from 'chokidar'
 import {deleteThemeAsset, fetchThemeAsset} from '@shopify/cli-kit/node/themes/api'
-import {outputDebug} from '@shopify/cli-kit/node/output'
+import {renderError} from '@shopify/cli-kit/node/ui'
 import EventEmitter from 'events'
 import type {Checksum, ThemeAsset} from '@shopify/cli-kit/node/themes/types'
 
@@ -563,7 +563,10 @@ describe('theme-fs', () => {
 
       // Then
       expect(deleteThemeAsset).toHaveBeenCalledWith(Number(themeId), 'assets/base.css', adminSession)
-      expect(outputDebug).toHaveBeenCalledWith('Failed to delete file "assets/base.css" from remote theme.')
+      expect(renderError).toHaveBeenCalledWith({
+        headline: 'Failed to delete file "assets/base.css" from remote theme.',
+        body: expect.any(String),
+      })
     })
   })
 

packages/theme/src/cli/utilities/theme-fs.ts

@@ -5,6 +5,7 @@ import {
   getPatternsFromShopifyIgnore,
 } from './asset-ignore.js'
 import {Notifier} from './notifier.js'
+import {createSyncingCatchError} from './errors.js'
 import {DEFAULT_IGNORE_PATTERNS, timestampDateFormat} from '../constants.js'
 import {glob, readFile, ReadOptions, fileExists, mkdir, writeFile, removeFile} from '@shopify/cli-kit/node/fs'
 import {joinPath, basename, relativePath} from '@shopify/cli-kit/node/path'
@@ -13,7 +14,6 @@ import {outputContent, outputDebug, outputInfo, outputToken, outputWarn} from '@
 import {buildThemeAsset} from '@shopify/cli-kit/node/themes/factories'
 import {AdminSession} from '@shopify/cli-kit/node/session'
 import {bulkUploadThemeAssets, deleteThemeAsset} from '@shopify/cli-kit/node/themes/api'
-import {renderError} from '@shopify/cli-kit/node/ui'
 import EventEmitter from 'node:events'
 import type {
   ThemeFileSystem,
@@ -144,27 +144,26 @@ export function mountThemeFileSystem(root: string, options?: ThemeFileSystemOpti
       return file.value || file.attachment || ''
     })
 
-    const syncPromise = contentPromise.then(async (content) => {
-      if (!unsyncedFileKeys.has(fileKey)) return false
+    const syncPromise = contentPromise
+      .then(async (content) => {
+        if (!unsyncedFileKeys.has(fileKey)) return false
 
-      const [result] = await bulkUploadThemeAssets(Number(themeId), [{key: fileKey, value: content}], adminSession)
+        const [result] = await bulkUploadThemeAssets(Number(themeId), [{key: fileKey, value: content}], adminSession)
+
+        if (!result?.success) {
+          throw new Error(
+            result?.errors?.asset
+              ? `\n\n${result.errors.asset.map((error) => `- ${error}`).join('\n')}`
+              : 'Response was not successful.',
+          )
+        }
 
-      if (result?.success) {
         unsyncedFileKeys.delete(fileKey)
         outputSyncResult('update', fileKey)
-      } else if (result?.errors?.asset) {
-        const errorMessage = `${fileKey}:\n${result.errors.asset.map((error) => `- ${error}`).join('\n')}`
-
-        renderError({
-          headline: 'Failed to sync file to remote theme.',
-          body: errorMessage,
-        })
-
-        return false
-      }
 
-      return true
-    })
+        return true
+      })
+      .catch(createSyncingCatchError(fileKey, 'upload'))
 
     emitEvent(eventName, {
       fileKey,
@@ -195,14 +194,13 @@ export function mountThemeFileSystem(root: string, options?: ThemeFileSystemOpti
     emitEvent('unlink', {fileKey})
 
     deleteThemeAsset(Number(themeId), fileKey, adminSession)
-      .then(async (success) => {
-        if (!success) throw new Error(`Failed to delete file "${fileKey}" from remote theme.`)
+      .then((success) => {
+        if (!success) throw new Error(`Response was not successful.`)
+
         unsyncedFileKeys.delete(fileKey)
         outputSyncResult('delete', fileKey)
       })
-      .catch((error) => {
-        outputDebug(error.message)
-      })
+      .catch(createSyncingCatchError(fileKey, 'delete'))
   }
 
   const directoriesToWatch = new Set(

packages/theme/src/cli/utilities/theme-uploader.ts

@@ -1,15 +1,17 @@
 import {partitionThemeFiles} from './theme-fs.js'
 import {rejectGeneratedStaticAssets} from './asset-checksum.js'
 import {renderTasksToStdErr} from './theme-ui.js'
+import {createSyncingCatchError} from './errors.js'
 import {AdminSession} from '@shopify/cli-kit/node/session'
 import {Result, Checksum, Theme, ThemeFileSystem} from '@shopify/cli-kit/node/themes/types'
 import {AssetParams, bulkUploadThemeAssets, deleteThemeAsset} from '@shopify/cli-kit/node/themes/api'
-import {renderError, renderWarning, Task} from '@shopify/cli-kit/node/ui'
+import {Task} from '@shopify/cli-kit/node/ui'
 import {outputDebug, outputInfo, outputNewline, outputWarn} from '@shopify/cli-kit/node/output'
 
 interface UploadOptions {
   nodelete?: boolean
   deferPartialWork?: boolean
+  backgroundWorkCatch?: (error: Error) => never
 }
 
 type ChecksumWithSize = Checksum & {size: number}
@@ -46,11 +48,16 @@ export function uploadTheme(
 
   const workPromise = options?.deferPartialWork
     ? themeCreationPromise
-    : deleteJobPromise
-        .then((result) => result.promise)
-        .catch(() => {
-          renderWarning({headline: 'Failed to delete outdated files from remote theme.'})
-        })
+    : deleteJobPromise.then((result) => result.promise)
+
+  if (options?.backgroundWorkCatch) {
+    // Aggregate all backgorund work in a single promise and handle errors
+    Promise.all([
+      themeCreationPromise,
+      uploadJobPromise.then((result) => result.promise),
+      deleteJobPromise.then((result) => result.promise),
+    ]).catch(options.backgroundWorkCatch)
+  }
 
   return {
     uploadResults,
@@ -124,12 +131,15 @@ function buildDeleteJob(
   const remoteFilesToBeDeleted = getRemoteFilesToBeDeleted(remoteChecksums, themeFileSystem)
   const orderedFiles = orderFilesToBeDeleted(remoteFilesToBeDeleted)
 
+  let failedDeleteAttempts = 0
   const progress = {current: 0, total: orderedFiles.length}
   const promise = Promise.all(
     orderedFiles.map((file) =>
       deleteThemeAsset(theme.id, file.key, session)
-        .catch((error) => {
-          renderError({headline: `Failed to delete file "${file.key}" from remote theme.`, body: error.message})
+        .catch((error: Error) => {
+          failedDeleteAttempts++
+          if (failedDeleteAttempts > 3) throw error
+          createSyncingCatchError(file.key, 'delete')(error)
         })
         .finally(() => {
           progress.current++