Merge pull request #6605 from Shopify/fix-theme-dev-failing-analytics

Fix theme dev failing analytics

Author: EvilGenius13

packages/theme/src/cli/polyfills/promiseWithResolvers.ts

@@ -0,0 +1,22 @@
+interface PromiseWithResolvers<T> {
+  promise: Promise<T>
+  resolve: (value: T | PromiseLike<T>) => void
+  reject: (reason?: unknown) => void
+}
+
+// Polyfill for Promise.withResolvers
+// Can remove once our minimum supported Node version is 22
+export function promiseWithResolvers<T>(): PromiseWithResolvers<T> {
+  if (typeof Promise.withResolvers === 'function') {
+    return Promise.withResolvers<T>()
+  }
+
+  let resolve!: (value: T | PromiseLike<T>) => void
+  let reject!: (reason?: unknown) => void
+  const promise = new Promise<T>((_resolve, _reject) => {
+    resolve = _resolve
+    reject = _reject
+  })
+
+  return {promise, resolve, reject}
+}

packages/theme/src/cli/services/dev.ts

@@ -119,23 +119,7 @@ export async function dev(options: DevOptions) {
     },
   }
 
-  if (options['theme-editor-sync']) {
-    session.storefrontPassword = await storefrontPasswordPromise
-  }
-
-  const {serverStart, renderDevSetupProgress} = setupDevServer(options.theme, ctx)
-
-  if (!options['theme-editor-sync']) {
-    session.storefrontPassword = await storefrontPasswordPromise
-  }
-
-  await renderDevSetupProgress()
-  await serverStart()
-
-  renderLinks(urls)
-  if (options.open) {
-    openURLSafely(urls.local, 'development server')
-  }
+  const {serverStart, renderDevSetupProgress, backgroundJobPromise} = setupDevServer(options.theme, ctx)
 
   readline.emitKeypressEvents(process.stdin)
   if (process.stdin.isTTY) {
@@ -167,6 +151,18 @@ export async function dev(options: DevOptions) {
         break
     }
   })
+
+  await Promise.all([
+    backgroundJobPromise,
+    renderDevSetupProgress()
+      .then(serverStart)
+      .then(() => {
+        renderLinks(urls)
+        if (options.open) {
+          openURLSafely(urls.local, 'development server')
+        }
+      }),
+  ])
 }
 
 export function openURLSafely(url: string, label: string) {

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

@@ -28,6 +28,7 @@ describe('reconcileAndPollThemeEditorChanges', async () => {
     const files = new Map<string, ThemeAsset>([])
     const defaultThemeFileSystem = fakeThemeFileSystem('tmp', files)
     const initialRemoteChecksums = [{checksum: '1', key: 'templates/asset.json'}]
+    const mockRejectBackgroundJob = vi.fn()
 
     vi.mocked(fetchChecksums).mockResolvedValue([{checksum: '2', key: 'templates/asset.json'}])
 
@@ -42,6 +43,7 @@ describe('reconcileAndPollThemeEditorChanges', async () => {
         ignore: [],
         only: [],
       },
+      mockRejectBackgroundJob,
     )
     await workPromise
     await updatedRemoteChecksumsPromise
@@ -57,6 +59,7 @@ describe('reconcileAndPollThemeEditorChanges', async () => {
         ignore: [],
         only: [],
       },
+      mockRejectBackgroundJob,
     )
   })
 
@@ -65,6 +68,7 @@ describe('reconcileAndPollThemeEditorChanges', async () => {
     const files = new Map<string, ThemeAsset>([])
     const defaultThemeFileSystem = fakeThemeFileSystem('tmp', files)
     const initialRemoteChecksums = [{checksum: '1', key: 'templates/asset.json'}]
+    const mockRejectBackgroundJob = vi.fn()
     const options = {
       noDelete: false,
       ignore: [],
@@ -83,6 +87,7 @@ describe('reconcileAndPollThemeEditorChanges', async () => {
       initialRemoteChecksums,
       defaultThemeFileSystem,
       options,
+      mockRejectBackgroundJob,
     )
 
     // Then

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

@@ -22,6 +22,7 @@ export async function reconcileAndPollThemeEditorChanges(
     ignore: string[]
     only: string[]
   },
+  rejectBackgroundJob: (reason?: unknown) => void,
 ): Promise<{
   updatedRemoteChecksumsPromise: Promise<Checksum[]>
   workPromise: Promise<void>
@@ -33,7 +34,14 @@ export async function reconcileAndPollThemeEditorChanges(
 
   const updatedRemoteChecksumsPromise = workPromise.then(async () => {
     const updatedRemoteChecksums = await fetchChecksums(targetTheme.id, session)
-    pollThemeEditorChanges(targetTheme, session, updatedRemoteChecksums, localThemeFileSystem, options)
+    pollThemeEditorChanges(
+      targetTheme,
+      session,
+      updatedRemoteChecksums,
+      localThemeFileSystem,
+      options,
+      rejectBackgroundJob,
+    )
     return updatedRemoteChecksums
   })
 

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

@@ -11,10 +11,11 @@ import {describe, expect, test, vi, beforeEach, afterEach} from 'vitest'
 import {buildTheme} from '@shopify/cli-kit/node/themes/factories'
 import {createEvent} from 'h3'
 import * as output from '@shopify/cli-kit/node/output'
+import {fetchChecksums} from '@shopify/cli-kit/node/themes/api'
 import {IncomingMessage, ServerResponse} from 'node:http'
 import {Socket} from 'node:net'
 
-vi.mock('@shopify/cli-kit/node/themes/api', () => ({fetchChecksums: () => Promise.resolve([])}))
+vi.mock('@shopify/cli-kit/node/themes/api', () => ({fetchChecksums: vi.fn(() => Promise.resolve([]))}))
 vi.mock('./remote-theme-watcher.js')
 vi.mock('./storefront-renderer.js')
 vi.spyOn(output, 'outputDebug')
@@ -158,7 +159,15 @@ describe('setupDevServer', () => {
       [],
       context.localThemeFileSystem,
       {noDelete: true, ...filters},
+      expect.anything(),
     )
+    // This is the best way I could think of verifying the rejectBackgroundJob
+    // Verify the rejectBackgroundJob callback is a function accepting one argument
+    const callArgs = vi.mocked(reconcileAndPollThemeEditorChanges).mock.calls[0]
+    const rejectCallback = callArgs?.[5]
+    expect(rejectCallback).toBeTypeOf('function')
+    // Reject callbacks take 1 argument: the rejection reason
+    expect(rejectCallback).toHaveLength(1)
   })
 
   test('should skip deletion of remote files if noDelete flag is passed', async () => {
@@ -179,6 +188,22 @@ describe('setupDevServer', () => {
     })
   })
 
+  test('should catch errors from fetchChecksums and reject backgroundJobPromise', async () => {
+    // Given
+    const context: DevServerContext = {
+      ...defaultServerContext,
+    }
+    const expectedError = new Error('Failed to fetch checksums from API')
+
+    vi.mocked(fetchChecksums).mockRejectedValueOnce(expectedError)
+
+    // When
+    const {backgroundJobPromise} = setupDevServer(developmentTheme, context)
+
+    // Then
+    await expect(backgroundJobPromise).rejects.toThrow('Failed to fetch checksums from API')
+  })
+
   describe('request handling', async () => {
     const context = {...defaultServerContext}
     const server = setupDevServer(developmentTheme, context)

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

@@ -5,16 +5,19 @@ 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 {renderThrownError} from '../errors.js'
+import {promiseWithResolvers} from '../../polyfills/promiseWithResolvers.js'
 import {createApp, defineEventHandler, defineLazyEventHandler, toNodeListener, handleCors} from 'h3'
 import {fetchChecksums} from '@shopify/cli-kit/node/themes/api'
 import {createServer} from 'node:http'
 import type {Checksum, Theme} from '@shopify/cli-kit/node/themes/types'
 import type {DevServerContext} from './types.js'
 
 export function setupDevServer(theme: Theme, ctx: DevServerContext) {
+  const {promise: backgroundJobPromise, reject: rejectBackgroundJob} = promiseWithResolvers<never>()
+
   const watcherPromise = setupInMemoryTemplateWatcher(theme, ctx)
-  const envSetup = ensureThemeEnvironmentSetup(theme, ctx)
+  const envSetup = ensureThemeEnvironmentSetup(theme, ctx, rejectBackgroundJob)
   const workPromise = Promise.all([watcherPromise, envSetup.workPromise]).then(() =>
     ctx.localThemeFileSystem.startWatcher(theme.id.toString(), ctx.session),
   )
@@ -25,31 +28,43 @@ export function setupDevServer(theme: Theme, ctx: DevServerContext) {
     serverStart: server.start,
     dispatchEvent: server.dispatch,
     renderDevSetupProgress: envSetup.renderProgress,
+    backgroundJobPromise,
   }
 }
 
-function ensureThemeEnvironmentSetup(theme: Theme, ctx: DevServerContext) {
-  const abort = createAbortCatchError('Failed to perform the initial theme synchronization.')
+function ensureThemeEnvironmentSetup(
+  theme: Theme,
+  ctx: DevServerContext,
+  rejectBackgroundJob: (reason?: unknown) => void,
+) {
+  const abort = (error: Error): never => {
+    renderThrownError('Failed to perform the initial theme synchronization.', error)
+    rejectBackgroundJob(error)
+    // Return a never-resolving promise to stop this promise chain without throwing.
+    // Throwing would trigger catch handlers and continue execution. This stops the
+    // chain while the error is handled through the separate backgroundJobPromise channel.
+    return new Promise<never>(() => {}) as never
+  }
 
-  const remoteChecksumsPromise = fetchChecksums(theme.id, ctx.session).catch(abort)
+  const remoteChecksumsPromise = fetchChecksums(theme.id, ctx.session)
 
-  const reconcilePromise = remoteChecksumsPromise
-    .then((remoteChecksums) => handleThemeEditorSync(theme, ctx, remoteChecksums))
-    .catch(abort)
+  const reconcilePromise = remoteChecksumsPromise.then((remoteChecksums) =>
+    handleThemeEditorSync(theme, ctx, remoteChecksums, rejectBackgroundJob),
+  )
 
-  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,
-      })
+  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)
+  })
+
+  const workPromise = uploadPromise.then((result) => result.workPromise).catch(abort)
 
   return {
-    workPromise: uploadPromise.then((result) => result.workPromise).catch(abort),
+    workPromise,
     renderProgress: async () => {
       if (ctx.options.themeEditorSync) {
         const {workPromise} = await reconcilePromise
@@ -74,16 +89,24 @@ function handleThemeEditorSync(
   theme: Theme,
   ctx: DevServerContext,
   remoteChecksums: Checksum[],
+  rejectBackgroundJob: (reason?: unknown) => void,
 ): Promise<{
   updatedRemoteChecksumsPromise: Promise<Checksum[]>
   workPromise: Promise<void>
 }> {
   if (ctx.options.themeEditorSync) {
-    return reconcileAndPollThemeEditorChanges(theme, ctx.session, remoteChecksums, ctx.localThemeFileSystem, {
-      noDelete: ctx.options.noDelete,
-      ignore: ctx.options.ignore,
-      only: ctx.options.only,
-    })
+    return reconcileAndPollThemeEditorChanges(
+      theme,
+      ctx.session,
+      remoteChecksums,
+      ctx.localThemeFileSystem,
+      {
+        noDelete: ctx.options.noDelete,
+        ignore: ctx.options.ignore,
+        only: ctx.options.only,
+      },
+      rejectBackgroundJob,
+    )
   } else {
     return Promise.resolve({
       updatedRemoteChecksumsPromise: Promise.resolve(remoteChecksums),

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

@@ -21,6 +21,7 @@ export function pollThemeEditorChanges(
   remoteChecksum: Checksum[],
   localFileSystem: ThemeFileSystem,
   options: PollingOptions,
+  rejectBackgroundJob: (reason?: unknown) => void,
 ) {
   outputDebug('Listening for changes in the theme editor')
 
@@ -51,14 +52,12 @@ export function pollThemeEditorChanges(
         }
 
         if (failedPollingAttempts >= maxPollingAttempts) {
-          renderFatalError(
-            new AbortError(
-              'Too many polling errors...',
-              'Please check the errors above and ensure you have a stable internet connection.',
-            ),
+          const fatalError = new AbortError(
+            'Too many polling errors...',
+            'Please check the errors above and ensure you have a stable internet connection.',
           )
-
-          process.exit(1)
+          renderFatalError(fatalError)
+          rejectBackgroundJob(fatalError)
         }
 
         return latestChecksums