Merge pull request #5964 from Shopify/bugfix/sync-images-theme-dev

Fix bug where `theme dev` doesnt sync images

Author: aswamy

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

@@ -710,40 +710,45 @@ describe('theme-fs', () => {
     test('returns false if file is not in unsyncedFileKeys', async () => {
       // Given
       unsyncedFileKeys = new Set()
-      const handler = handleSyncUpdate(unsyncedFileKeys, uploadErrors, fileKey, themeId, adminSession)!
+      const handler = handleSyncUpdate(unsyncedFileKeys, uploadErrors, fileKey, themeId, adminSession)
 
       // When
-      const result = await handler('content')
+      const result = await handler({value: 'content'})
 
       // Then
       expect(result).toBe(false)
       expect(bulkUploadThemeAssets).not.toHaveBeenCalled()
       expect(triggerBrowserFullReload).not.toHaveBeenCalled()
     })
 
-    test('uploads file and returns true on successful sync', async () => {
-      // Given
-      vi.mocked(bulkUploadThemeAssets).mockResolvedValue([
-        {
-          key: fileKey,
-          success: true,
-          operation: Operation.Upload,
-        },
-      ])
-      const handler = handleSyncUpdate(unsyncedFileKeys, uploadErrors, fileKey, themeId, adminSession)!
-
-      // When
-      const result = await handler('content')
-
-      // Then
-      expect(result).toBe(true)
-      expect(bulkUploadThemeAssets).toHaveBeenCalledWith(
-        Number(themeId),
-        [{key: fileKey, value: 'content'}],
-        adminSession,
-      )
-      expect(unsyncedFileKeys.has(fileKey)).toBe(false)
-      expect(triggerBrowserFullReload).not.toHaveBeenCalled()
+    Object.entries({
+      text: {value: 'content'},
+      image: {attachment: 'content'},
+    }).forEach(([fileType, fileContent]) => {
+      test(`uploads ${fileType} file and returns true on successful sync`, async () => {
+        // Given
+        vi.mocked(bulkUploadThemeAssets).mockResolvedValue([
+          {
+            key: fileKey,
+            success: true,
+            operation: Operation.Upload,
+          },
+        ])
+        const handler = handleSyncUpdate(unsyncedFileKeys, uploadErrors, fileKey, themeId, adminSession)
+
+        // When
+        const result = await handler(fileContent)
+
+        // Then
+        expect(result).toBe(true)
+        expect(bulkUploadThemeAssets).toHaveBeenCalledWith(
+          Number(themeId),
+          [{key: fileKey, ...fileContent}],
+          adminSession,
+        )
+        expect(unsyncedFileKeys.has(fileKey)).toBe(false)
+        expect(triggerBrowserFullReload).not.toHaveBeenCalled()
+      })
     })
 
     test('throws error and sets uploadErrors on failed sync', async () => {
@@ -757,10 +762,10 @@ describe('theme-fs', () => {
           errors: {asset: errors},
         },
       ])
-      const handler = handleSyncUpdate(unsyncedFileKeys, uploadErrors, fileKey, themeId, adminSession)!
+      const handler = handleSyncUpdate(unsyncedFileKeys, uploadErrors, fileKey, themeId, adminSession)
 
       // When/Then
-      await expect(handler('content')).rejects.toThrow('{{ broken liquid file')
+      await expect(handler({value: 'content'})).rejects.toThrow('{{ broken liquid file')
       expect(uploadErrors.get(fileKey)).toEqual(errors)
       expect(unsyncedFileKeys.has(fileKey)).toBe(true)
       expect(triggerBrowserFullReload).toHaveBeenCalledWith(themeId, fileKey)
@@ -776,10 +781,10 @@ describe('theme-fs', () => {
           operation: Operation.Upload,
         },
       ])
-      const handler = handleSyncUpdate(unsyncedFileKeys, uploadErrors, fileKey, themeId, adminSession)!
+      const handler = handleSyncUpdate(unsyncedFileKeys, uploadErrors, fileKey, themeId, adminSession)
 
       // When
-      await handler('content')
+      await handler({value: 'content'})
 
       // Then
       expect(uploadErrors.has(fileKey)).toBe(false)

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

@@ -139,21 +139,28 @@ export function mountThemeFileSystem(root: string, options?: ThemeFileSystemOpti
       const file = files.get(fileKey)
 
       if (!file) {
-        return ''
+        return
       }
 
       if (file.checksum !== previousChecksum) {
         // Sync only if the file has changed
         unsyncedFileKeys.add(fileKey)
       }
 
-      // file.value has a fallback value of '', so we want to ignore this eslint rule
-      // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
-      return file.value || file.attachment || ''
+      return {
+        // file.value has a fallback value of '', so we want to ignore this eslint rule
+        // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
+        value: file.value || undefined,
+        attachment: file.attachment,
+      }
     })
 
     const syncPromise = contentPromise
-      .then(handleSyncUpdate(unsyncedFileKeys, uploadErrors, fileKey, themeId, adminSession))
+      .then((content) => {
+        if (!content) return
+
+        return handleSyncUpdate(unsyncedFileKeys, uploadErrors, fileKey, themeId, adminSession)(content)
+      })
       .catch(createSyncingCatchError(fileKey, 'upload'))
 
     emitEvent(eventName, {
@@ -162,7 +169,7 @@ export function mountThemeFileSystem(root: string, options?: ThemeFileSystemOpti
         contentPromise
           .then((content) => {
             // Run only if content has changed
-            if (unsyncedFileKeys.has(fileKey)) fn(content)
+            if (unsyncedFileKeys.has(fileKey)) fn(content?.value ?? content?.attachment ?? '')
           })
           .catch(() => {})
       },
@@ -275,13 +282,13 @@ export function handleSyncUpdate(
   fileKey: string,
   themeId: string,
   adminSession: AdminSession,
-): ((value: string) => boolean | PromiseLike<boolean>) | null | undefined {
+): (content: {value?: string; attachment?: string}) => PromiseLike<boolean> {
   return 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, ...content}], adminSession)
 
     if (!result?.success) {
       const errors = result?.errors?.asset ?? ['Response was not successful.']