From 6edd52f1e01bfbd833749705e8e34b9cb16ec24d Mon Sep 17 00:00:00 2001 From: James Meng Date: Thu, 16 Jan 2025 12:18:08 -0800 Subject: [PATCH] [Themes] Store theme asset upload errors in the themeFileSystem --- .../cli-kit/src/public/node/themes/types.ts | 5 ++ .../theme/src/cli/utilities/theme-fs-empty.ts | 1 + .../theme/src/cli/utilities/theme-fs.test.ts | 2 + packages/theme/src/cli/utilities/theme-fs.ts | 14 ++-- .../theme-fs/theme-fs-mock-factory.ts | 1 + .../src/cli/utilities/theme-uploader.test.ts | 81 +++++++++++++++++++ .../theme/src/cli/utilities/theme-uploader.ts | 10 +++ 7 files changed, 108 insertions(+), 6 deletions(-) diff --git a/packages/cli-kit/src/public/node/themes/types.ts b/packages/cli-kit/src/public/node/themes/types.ts index 1fdf1e6d2eb..ef2786c7d0e 100644 --- a/packages/cli-kit/src/public/node/themes/types.ts +++ b/packages/cli-kit/src/public/node/themes/types.ts @@ -99,6 +99,11 @@ export interface ThemeFileSystem extends VirtualFileSystem { * Applies filters to ignore files from .shopifyignore file, --ignore and --only flags. */ applyIgnoreFilters: (files: T[]) => T[] + + /** + * Stores upload errors returned when uploading files via the Asset API + */ + uploadErrors: Map } /** diff --git a/packages/theme/src/cli/utilities/theme-fs-empty.ts b/packages/theme/src/cli/utilities/theme-fs-empty.ts index 51c40f4520d..126170fb0b3 100644 --- a/packages/theme/src/cli/utilities/theme-fs-empty.ts +++ b/packages/theme/src/cli/utilities/theme-fs-empty.ts @@ -13,6 +13,7 @@ function emptyFileSystem(): T { root: '', files: new Map(), unsyncedFileKeys: new Set(), + uploadErrors: new Map(), ready: () => Promise.resolve(), delete: async (_: string) => {}, read: async (_: string) => '', diff --git a/packages/theme/src/cli/utilities/theme-fs.test.ts b/packages/theme/src/cli/utilities/theme-fs.test.ts index 9b09633dc77..f0a81010ac9 100644 --- a/packages/theme/src/cli/utilities/theme-fs.test.ts +++ b/packages/theme/src/cli/utilities/theme-fs.test.ts @@ -59,6 +59,7 @@ describe('theme-fs', () => { fsEntry({checksum: '64caf742bd427adcf497bffab63df30c', key: 'templates/404.json'}), ]), unsyncedFileKeys: new Set(), + uploadErrors: new Map(), ready: expect.any(Function), delete: expect.any(Function), write: expect.any(Function), @@ -82,6 +83,7 @@ describe('theme-fs', () => { root, files: new Map(), unsyncedFileKeys: new Set(), + uploadErrors: new Map(), ready: expect.any(Function), delete: expect.any(Function), write: expect.any(Function), diff --git a/packages/theme/src/cli/utilities/theme-fs.ts b/packages/theme/src/cli/utilities/theme-fs.ts index 820a8f68724..93e018b497f 100644 --- a/packages/theme/src/cli/utilities/theme-fs.ts +++ b/packages/theme/src/cli/utilities/theme-fs.ts @@ -45,6 +45,7 @@ const THEME_PARTITION_REGEX = { export function mountThemeFileSystem(root: string, options?: ThemeFileSystemOptions): ThemeFileSystem { const files = new Map() + const uploadErrors = new Map() const unsyncedFileKeys = new Set() const filterPatterns = { ignoreFromFile: [] as string[], @@ -147,12 +148,12 @@ export function mountThemeFileSystem(root: string, options?: ThemeFileSystemOpti 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) { + uploadErrors.delete(fileKey) + } else { + const errors = result?.errors?.asset ?? ['Response was not successful.'] + uploadErrors.set(fileKey, errors) + throw new Error(errors.join('\n')) } unsyncedFileKeys.delete(fileKey) @@ -223,6 +224,7 @@ export function mountThemeFileSystem(root: string, options?: ThemeFileSystemOpti root, files, unsyncedFileKeys, + uploadErrors, ready: () => themeSetupPromise, delete: async (fileKey: string) => { files.delete(fileKey) diff --git a/packages/theme/src/cli/utilities/theme-fs/theme-fs-mock-factory.ts b/packages/theme/src/cli/utilities/theme-fs/theme-fs-mock-factory.ts index ce559833cb8..e870c8d190c 100644 --- a/packages/theme/src/cli/utilities/theme-fs/theme-fs-mock-factory.ts +++ b/packages/theme/src/cli/utilities/theme-fs/theme-fs-mock-factory.ts @@ -10,6 +10,7 @@ export function fakeThemeFileSystem( root, files, unsyncedFileKeys: new Set(), + uploadErrors: new Map(), ready: () => Promise.resolve(), delete: async (fileKey: string) => { files.delete(fileKey) diff --git a/packages/theme/src/cli/utilities/theme-uploader.test.ts b/packages/theme/src/cli/utilities/theme-uploader.test.ts index eed1e2cae81..b16b27b8d6a 100644 --- a/packages/theme/src/cli/utilities/theme-uploader.test.ts +++ b/packages/theme/src/cli/utilities/theme-uploader.test.ts @@ -4,6 +4,7 @@ import { MAX_UPLOAD_RETRY_COUNT, MINIMUM_THEME_ASSETS, uploadTheme, + updateUploadErrors, } from './theme-uploader.js' import {fakeThemeFileSystem} from './theme-fs/theme-fs-mock-factory.js' import {bulkUploadThemeAssets, deleteThemeAsset} from '@shopify/cli-kit/node/themes/api' @@ -602,3 +603,83 @@ describe('theme-uploader', () => { ) }) }) + +describe('updateUploadErrors', () => { + test('should clear error for successful upload', () => { + // Given + const themeFileSystem = fakeThemeFileSystem('tmp', new Map()) + const result: Result = { + key: 'file1.liquid', + success: true, + errors: {}, + operation: Operation.Upload, + asset: {key: 'file1.liquid', checksum: 'abc'}, + } + + // Pre-existing error that should be cleared + themeFileSystem.uploadErrors.set('file1.liquid', ['Old error']) + + // When + updateUploadErrors(result, themeFileSystem) + + // Then + expect(themeFileSystem.uploadErrors.has('file1.liquid')).toBe(false) + }) + + test('should set error for failed upload', () => { + // Given + const themeFileSystem = fakeThemeFileSystem('tmp', new Map()) + const result: Result = { + key: 'file1.liquid', + success: false, + errors: {asset: ['Upload failed']}, + operation: Operation.Upload, + asset: {key: 'file1.liquid', checksum: 'abc'}, + } + + // When + updateUploadErrors(result, themeFileSystem) + + // Then + expect(themeFileSystem.uploadErrors.get('file1.liquid')).toEqual(['Upload failed']) + }) + + test('should set default error when no specific error provided', () => { + // Given + const themeFileSystem = fakeThemeFileSystem('tmp', new Map()) + const result: Result = { + key: 'file1.liquid', + success: false, + errors: {}, + operation: Operation.Upload, + asset: {key: 'file1.liquid', checksum: 'abc'}, + } + + // When + updateUploadErrors(result, themeFileSystem) + + // Then + expect(themeFileSystem.uploadErrors.get('file1.liquid')).toEqual(['Response was not successful.']) + }) + + test('should update existing error', () => { + // Given + const themeFileSystem = fakeThemeFileSystem('tmp', new Map()) + const result: Result = { + key: 'file1.liquid', + success: false, + errors: {asset: ['New error']}, + operation: Operation.Upload, + asset: {key: 'file1.liquid', checksum: 'abc'}, + } + + // Pre-existing error that should be updated + themeFileSystem.uploadErrors.set('file1.liquid', ['Old error']) + + // When + updateUploadErrors(result, themeFileSystem) + + // Then + expect(themeFileSystem.uploadErrors.get('file1.liquid')).toEqual(['New error']) + }) +}) diff --git a/packages/theme/src/cli/utilities/theme-uploader.ts b/packages/theme/src/cli/utilities/theme-uploader.ts index d2e9c6e2c7d..59fa7f83758 100644 --- a/packages/theme/src/cli/utilities/theme-uploader.ts +++ b/packages/theme/src/cli/utilities/theme-uploader.ts @@ -370,9 +370,19 @@ async function uploadBatch( // store the results in uploadResults, overwriting any existing results results.forEach((result) => { uploadResults.set(result.key, result) + updateUploadErrors(result, localThemeFileSystem) }) } +export function updateUploadErrors(result: Result, localThemeFileSystem: ThemeFileSystem) { + if (result.success) { + localThemeFileSystem.uploadErrors.delete(result.key) + } else { + const errors = result.errors?.asset ?? ['Response was not successful.'] + localThemeFileSystem.uploadErrors.set(result.key, errors) + } +} + async function handleBulkUpload( uploadParams: AssetParams[], themeId: number,