From fc9d92b7e3520fb8cb53fec92e4c7bb16d299b2f Mon Sep 17 00:00:00 2001 From: Chris AtLee Date: Tue, 5 Nov 2024 17:07:08 -0500 Subject: [PATCH] Port theme file deletes to graphQL --- .../admin/generated/theme_files_delete.ts | 100 ++++++++++++++++++ .../api/graphql/admin/generated/types.d.ts | 19 ++++ .../mutations/theme_files_delete.graphql | 12 +++ .../src/public/node/themes/api.test.ts | 62 +++++------ .../cli-kit/src/public/node/themes/api.ts | 47 +++++++- .../theme-reconciliation.test.ts | 4 +- .../theme-environment/theme-reconciliation.ts | 10 +- .../theme/src/cli/utilities/theme-fs.test.ts | 22 ++-- packages/theme/src/cli/utilities/theme-fs.ts | 8 +- .../src/cli/utilities/theme-uploader.test.ts | 41 +++---- .../theme/src/cli/utilities/theme-uploader.ts | 44 +++++--- 11 files changed, 274 insertions(+), 95 deletions(-) create mode 100644 packages/cli-kit/src/cli/api/graphql/admin/generated/theme_files_delete.ts create mode 100644 packages/cli-kit/src/cli/api/graphql/admin/mutations/theme_files_delete.graphql diff --git a/packages/cli-kit/src/cli/api/graphql/admin/generated/theme_files_delete.ts b/packages/cli-kit/src/cli/api/graphql/admin/generated/theme_files_delete.ts new file mode 100644 index 00000000000..f451cddab4a --- /dev/null +++ b/packages/cli-kit/src/cli/api/graphql/admin/generated/theme_files_delete.ts @@ -0,0 +1,100 @@ +/* eslint-disable @typescript-eslint/consistent-type-definitions */ +import * as Types from './types.js' + +import {TypedDocumentNode as DocumentNode} from '@graphql-typed-document-node/core' + +export type ThemeFilesDeleteMutationVariables = Types.Exact<{ + themeId: Types.Scalars['ID']['input'] + files: Types.Scalars['String']['input'][] | Types.Scalars['String']['input'] +}> + +export type ThemeFilesDeleteMutation = { + themeFilesDelete?: { + deletedThemeFiles?: {filename: string}[] | null + userErrors: { + filename?: string | null + code?: Types.OnlineStoreThemeFilesUserErrorsCode | null + message: string + }[] + } | null +} + +export const ThemeFilesDelete = { + kind: 'Document', + definitions: [ + { + kind: 'OperationDefinition', + operation: 'mutation', + name: {kind: 'Name', value: 'themeFilesDelete'}, + variableDefinitions: [ + { + kind: 'VariableDefinition', + variable: {kind: 'Variable', name: {kind: 'Name', value: 'themeId'}}, + type: {kind: 'NonNullType', type: {kind: 'NamedType', name: {kind: 'Name', value: 'ID'}}}, + }, + { + kind: 'VariableDefinition', + variable: {kind: 'Variable', name: {kind: 'Name', value: 'files'}}, + type: { + kind: 'NonNullType', + type: { + kind: 'ListType', + type: {kind: 'NonNullType', type: {kind: 'NamedType', name: {kind: 'Name', value: 'String'}}}, + }, + }, + }, + ], + selectionSet: { + kind: 'SelectionSet', + selections: [ + { + kind: 'Field', + name: {kind: 'Name', value: 'themeFilesDelete'}, + arguments: [ + { + kind: 'Argument', + name: {kind: 'Name', value: 'themeId'}, + value: {kind: 'Variable', name: {kind: 'Name', value: 'themeId'}}, + }, + { + kind: 'Argument', + name: {kind: 'Name', value: 'files'}, + value: {kind: 'Variable', name: {kind: 'Name', value: 'files'}}, + }, + ], + selectionSet: { + kind: 'SelectionSet', + selections: [ + { + kind: 'Field', + name: {kind: 'Name', value: 'deletedThemeFiles'}, + selectionSet: { + kind: 'SelectionSet', + selections: [ + {kind: 'Field', name: {kind: 'Name', value: 'filename'}}, + {kind: 'Field', name: {kind: 'Name', value: '__typename'}}, + ], + }, + }, + { + kind: 'Field', + name: {kind: 'Name', value: 'userErrors'}, + selectionSet: { + kind: 'SelectionSet', + selections: [ + {kind: 'Field', name: {kind: 'Name', value: 'filename'}}, + {kind: 'Field', name: {kind: 'Name', value: 'code'}}, + {kind: 'Field', name: {kind: 'Name', value: 'message'}}, + {kind: 'Field', name: {kind: 'Name', value: '__typename'}}, + ], + }, + }, + {kind: 'Field', name: {kind: 'Name', value: '__typename'}}, + ], + }, + }, + ], + }, + }, + ], +} as unknown as DocumentNode diff --git a/packages/cli-kit/src/cli/api/graphql/admin/generated/types.d.ts b/packages/cli-kit/src/cli/api/graphql/admin/generated/types.d.ts index 65f693bfcd2..68a999b690c 100644 --- a/packages/cli-kit/src/cli/api/graphql/admin/generated/types.d.ts +++ b/packages/cli-kit/src/cli/api/graphql/admin/generated/types.d.ts @@ -162,6 +162,25 @@ export type OnlineStoreThemeFilesUpsertFileInput = { filename: Scalars['String']['input'] } +/** Possible error codes that can be returned by `OnlineStoreThemeFilesUserErrors`. */ +export type OnlineStoreThemeFilesUserErrorsCode = + /** Access denied. */ + | 'ACCESS_DENIED' + /** There are files with the same filename. */ + | 'DUPLICATE_FILE_INPUT' + /** Error. */ + | 'ERROR' + /** The file is invalid. */ + | 'FILE_VALIDATION_ERROR' + /** The input value should be less than or equal to the maximum value allowed. */ + | 'LESS_THAN_OR_EQUAL_TO' + /** The record with the ID used as the input value couldn't be found. */ + | 'NOT_FOUND' + /** There are theme files with conflicts. */ + | 'THEME_FILES_CONFLICT' + /** This action is not available on your current plan. Please upgrade to access theme editing features. */ + | 'THEME_LIMITED_PLAN' + /** The input fields for Theme attributes to update. */ export type OnlineStoreThemeInput = { /** The new name of the theme. */ diff --git a/packages/cli-kit/src/cli/api/graphql/admin/mutations/theme_files_delete.graphql b/packages/cli-kit/src/cli/api/graphql/admin/mutations/theme_files_delete.graphql new file mode 100644 index 00000000000..0d457035ffd --- /dev/null +++ b/packages/cli-kit/src/cli/api/graphql/admin/mutations/theme_files_delete.graphql @@ -0,0 +1,12 @@ +mutation themeFilesDelete($themeId: ID!, $files: [String!]!) { + themeFilesDelete(themeId: $themeId, files: $files) { + deletedThemeFiles { + filename + } + userErrors { + filename + code + message + } + } +} diff --git a/packages/cli-kit/src/public/node/themes/api.test.ts b/packages/cli-kit/src/public/node/themes/api.test.ts index e74ba0159ba..8e74844a885 100644 --- a/packages/cli-kit/src/public/node/themes/api.test.ts +++ b/packages/cli-kit/src/public/node/themes/api.test.ts @@ -8,7 +8,7 @@ import { fetchChecksums, bulkUploadThemeAssets, AssetParams, - deleteThemeAsset, + deleteThemeAssets, } from './api.js' import {Operation} from './types.js' import {ThemeDelete} from '../../../cli/api/graphql/admin/generated/theme_delete.js' @@ -16,6 +16,7 @@ import {ThemeUpdate} from '../../../cli/api/graphql/admin/generated/theme_update import {ThemePublish} from '../../../cli/api/graphql/admin/generated/theme_publish.js' import {GetThemeFileChecksums} from '../../../cli/api/graphql/admin/generated/get_theme_file_checksums.js' import {ThemeFilesUpsert} from '../../../cli/api/graphql/admin/generated/theme_files_upsert.js' +import {ThemeFilesDelete} from '../../../cli/api/graphql/admin/generated/theme_files_delete.js' import {OnlineStoreThemeFileBodyInputType} from '../../../cli/api/graphql/admin/generated/types.js' import {test, vi, expect, describe} from 'vitest' import {adminRequestDoc, restRequest, supportedApiVersions} from '@shopify/cli-kit/node/api/admin' @@ -269,63 +270,51 @@ describe('themePublish', () => { } }) -describe('deleteThemeAsset', () => { +describe('deleteThemeAssets', () => { test('deletes a theme asset', async () => { // Given const id = 123 const key = 'snippets/product-variant-picker.liquid' - vi.mocked(restRequest).mockResolvedValue({ - json: {message: 'snippets/product-variant-picker.liquid was succesfully deleted'}, - status: 200, - headers: {}, + vi.mocked(adminRequestDoc).mockResolvedValue({ + themeFilesDelete: { + deletedThemeFiles: [{filename: key}], + userErrors: [], + }, }) // When - const output = await deleteThemeAsset(id, key, session) + const output = await deleteThemeAssets(id, [key], session) // Then - expect(restRequest).toHaveBeenCalledWith('DELETE', `/themes/${id}/assets`, session, undefined, {'asset[key]': key}) - expect(output).toBe(true) + expect(adminRequestDoc).toHaveBeenCalledWith(ThemeFilesDelete, session, { + themeId: `gid://shopify/OnlineStoreTheme/${id}`, + files: [key], + }) + expect(output).toEqual([{key, success: true, operation: 'DELETE'}]) }) - test('returns true when attemping to delete an nonexistent asset', async () => { + test('returns success when attempting to delete nonexistent assets', async () => { // Given const id = 123 const key = 'snippets/product-variant-picker.liquid' - vi.mocked(restRequest).mockResolvedValue({ - json: {}, - status: 200, - headers: {}, + vi.mocked(adminRequestDoc).mockResolvedValue({ + themeFilesDelete: { + deletedThemeFiles: [{filename: key}], + userErrors: [], + }, }) // When - const output = await deleteThemeAsset(id, key, session) + const output = await deleteThemeAssets(id, [key], session) // Then - expect(restRequest).toHaveBeenCalledWith('DELETE', `/themes/${id}/assets`, session, undefined, {'asset[key]': key}) - expect(output).toBe(true) - }) - - test('throws an AbortError when the server responds with a 403', async () => { - // Given - const id = 123 - const key = 'config/settings_data.json' - const message = 'You are not authorized to edit themes on "my-shop.myshopify.com".' - - vi.mocked(restRequest).mockResolvedValue({ - json: {message}, - status: 403, - headers: {}, + expect(adminRequestDoc).toHaveBeenCalledWith(ThemeFilesDelete, session, { + themeId: `gid://shopify/OnlineStoreTheme/${id}`, + files: [key], }) - - // When - const deletePromise = () => deleteThemeAsset(id, key, session) - - // Then - await expect(deletePromise).rejects.toThrow(new AbortError(message)) - expect(restRequest).toHaveBeenCalledWith('DELETE', `/themes/${id}/assets`, session, undefined, {'asset[key]': key}) + expect(output).toEqual([{key, success: true, operation: 'DELETE'}]) }) }) @@ -334,7 +323,6 @@ describe('themeDelete', () => { test(`deletes a theme with graphql with a ${sessionType} session`, async () => { // Given const id = 123 - const name = 'store theme' vi.mocked(adminRequestDoc).mockResolvedValue({ themeDelete: { diff --git a/packages/cli-kit/src/public/node/themes/api.ts b/packages/cli-kit/src/public/node/themes/api.ts index 23966667812..1829dfa3155 100644 --- a/packages/cli-kit/src/public/node/themes/api.ts +++ b/packages/cli-kit/src/public/node/themes/api.ts @@ -10,6 +10,7 @@ import { ThemeFilesUpsert, ThemeFilesUpsertMutation, } from '../../../cli/api/graphql/admin/generated/theme_files_upsert.js' +import {ThemeFilesDelete} from '../../../cli/api/graphql/admin/generated/theme_files_delete.js' import { OnlineStoreThemeFileBodyInputType, OnlineStoreThemeFilesUpsertFileInput, @@ -94,11 +95,47 @@ export async function fetchThemeAssets(id: number, filenames: Key[], session: Ad } } -export async function deleteThemeAsset(id: number, key: Key, session: AdminSession): Promise { - const response = await request('DELETE', `/themes/${id}/assets`, session, undefined, { - 'asset[key]': key, - }) - return response.status === 200 +export async function deleteThemeAssets(id: number, filenames: Key[], session: AdminSession): Promise { + const batchSize = 50 + const results: Result[] = [] + + for (let i = 0; i < filenames.length; i += batchSize) { + const batch = filenames.slice(i, i + batchSize) + // eslint-disable-next-line no-await-in-loop + const {themeFilesDelete} = await adminRequestDoc(ThemeFilesDelete, session, { + themeId: composeThemeGid(id), + files: batch, + }) + + if (!themeFilesDelete) { + unexpectedGraphQLError('Failed to delete theme assets') + } + + const {deletedThemeFiles, userErrors} = themeFilesDelete + + if (deletedThemeFiles) { + deletedThemeFiles.forEach((file) => { + results.push({key: file.filename, success: true, operation: Operation.Delete}) + }) + } + + if (userErrors.length > 0) { + userErrors.forEach((error) => { + if (error.filename) { + results.push({ + key: error.filename, + success: false, + operation: Operation.Delete, + errors: {asset: [error.message]}, + }) + } else { + unexpectedGraphQLError(`Failed to delete theme assets: ${error.message}`) + } + }) + } + } + + return results } export async function bulkUploadThemeAssets( diff --git a/packages/theme/src/cli/utilities/theme-environment/theme-reconciliation.test.ts b/packages/theme/src/cli/utilities/theme-environment/theme-reconciliation.test.ts index e20e8b88976..0083c373b40 100644 --- a/packages/theme/src/cli/utilities/theme-environment/theme-reconciliation.test.ts +++ b/packages/theme/src/cli/utilities/theme-environment/theme-reconciliation.test.ts @@ -1,7 +1,7 @@ import {reconcileJsonFiles} from './theme-reconciliation.js' import {REMOTE_STRATEGY, LOCAL_STRATEGY} from './remote-theme-watcher.js' import {fakeThemeFileSystem} from '../theme-fs/theme-fs-mock-factory.js' -import {deleteThemeAsset, fetchThemeAssets} from '@shopify/cli-kit/node/themes/api' +import {deleteThemeAssets, fetchThemeAssets} from '@shopify/cli-kit/node/themes/api' import {buildTheme} from '@shopify/cli-kit/node/themes/factories' import {Checksum, ThemeAsset, ThemeFileSystem} from '@shopify/cli-kit/node/themes/types' import {DEVELOPMENT_THEME_ROLE} from '@shopify/cli-kit/node/themes/utils' @@ -143,7 +143,7 @@ describe('reconcileJsonFiles', () => { ) // Then - expect(deleteThemeAsset).toHaveBeenCalledWith(developmentTheme.id, assetToBeDeleted.key, adminSession) + expect(deleteThemeAssets).toHaveBeenCalledWith(developmentTheme.id, [assetToBeDeleted.key], adminSession) expect(defaultThemeFileSystem.files.get('templates/asset.json')).toBeUndefined() }) }) diff --git a/packages/theme/src/cli/utilities/theme-environment/theme-reconciliation.ts b/packages/theme/src/cli/utilities/theme-environment/theme-reconciliation.ts index 7d973f4a083..07527c27ad4 100644 --- a/packages/theme/src/cli/utilities/theme-environment/theme-reconciliation.ts +++ b/packages/theme/src/cli/utilities/theme-environment/theme-reconciliation.ts @@ -3,7 +3,7 @@ import {batchedRequests} from '../batching.js' import {MAX_GRAPHQL_THEME_FILES} from '../../constants.js' import {outputDebug} from '@shopify/cli-kit/node/output' import {AdminSession} from '@shopify/cli-kit/node/session' -import {deleteThemeAsset, fetchThemeAssets} from '@shopify/cli-kit/node/themes/api' +import {deleteThemeAssets, fetchThemeAssets} from '@shopify/cli-kit/node/themes/api' import {Checksum, ThemeFileSystem, ThemeAsset, Theme} from '@shopify/cli-kit/node/themes/types' import {renderInfo, renderSelectPrompt} from '@shopify/cli-kit/node/ui' @@ -179,9 +179,13 @@ async function performFileReconciliation( ) }) - const deleteRemoteFiles = remoteFilesToDelete.map((file) => deleteThemeAsset(targetTheme.id, file.key, session)) + const deleteRemoteFiles = deleteThemeAssets( + targetTheme.id, + remoteFilesToDelete.map((file) => file.key), + session, + ) - await Promise.all([...deleteLocalFiles, ...downloadRemoteFiles, ...deleteRemoteFiles]) + await Promise.all([...deleteLocalFiles, ...downloadRemoteFiles, deleteRemoteFiles]) } async function partitionFilesByReconciliationStrategy( diff --git a/packages/theme/src/cli/utilities/theme-fs.test.ts b/packages/theme/src/cli/utilities/theme-fs.test.ts index 09b2b386ff4..c649549afa8 100644 --- a/packages/theme/src/cli/utilities/theme-fs.test.ts +++ b/packages/theme/src/cli/utilities/theme-fs.test.ts @@ -11,10 +11,10 @@ import {getPatternsFromShopifyIgnore, applyIgnoreFilters} from './asset-ignore.j import {removeFile, writeFile} from '@shopify/cli-kit/node/fs' import {test, describe, expect, vi, beforeEach} from 'vitest' import chokidar from 'chokidar' -import {deleteThemeAsset, fetchThemeAssets} from '@shopify/cli-kit/node/themes/api' +import {deleteThemeAssets, fetchThemeAssets} from '@shopify/cli-kit/node/themes/api' import {renderError} from '@shopify/cli-kit/node/ui' +import {Operation, type Checksum, type ThemeAsset} from '@shopify/cli-kit/node/themes/types' import EventEmitter from 'events' -import type {Checksum, ThemeAsset} from '@shopify/cli-kit/node/themes/types' vi.mock('@shopify/cli-kit/node/fs', async (realImport) => { const realModule = await realImport() @@ -508,7 +508,9 @@ describe('theme-fs', () => { stats: {size: 100, mtime: 100}, }, ]) - vi.mocked(deleteThemeAsset).mockResolvedValue(true) + vi.mocked(deleteThemeAssets).mockResolvedValue([ + {key: 'assets/base.css', success: true, operation: Operation.Delete}, + ]) // When const themeFileSystem = mountThemeFileSystem(root) @@ -529,7 +531,7 @@ describe('theme-fs', () => { await deleteOperationPromise // Then - expect(deleteThemeAsset).toHaveBeenCalledWith(Number(themeId), 'assets/base.css', adminSession) + expect(deleteThemeAssets).toHaveBeenCalledWith(Number(themeId), ['assets/base.css'], adminSession) }) test('does not delete file from remote when options.noDelete is true', async () => { @@ -543,7 +545,9 @@ describe('theme-fs', () => { stats: {size: 100, mtime: 100}, }, ]) - vi.mocked(deleteThemeAsset).mockResolvedValue(true) + vi.mocked(deleteThemeAssets).mockResolvedValue([ + {key: 'assets/base.css', success: true, operation: Operation.Delete}, + ]) // When const themeFileSystem = mountThemeFileSystem(root, {noDelete: true}) @@ -564,7 +568,7 @@ describe('theme-fs', () => { await deleteOperationPromise // Then - expect(deleteThemeAsset).not.toHaveBeenCalled() + expect(deleteThemeAssets).not.toHaveBeenCalled() }) test('renders a warning to debug if the file deletion fails', async () => { @@ -577,7 +581,9 @@ describe('theme-fs', () => { stats: {size: 100, mtime: 100}, }, ]) - vi.mocked(deleteThemeAsset).mockResolvedValue(false) + vi.mocked(deleteThemeAssets).mockResolvedValue([ + {key: 'assets/base.css', success: false, operation: Operation.Delete}, + ]) // When const themeFileSystem = mountThemeFileSystem(root) @@ -598,7 +604,7 @@ describe('theme-fs', () => { await deleteOperationPromise // Then - expect(deleteThemeAsset).toHaveBeenCalledWith(Number(themeId), 'assets/base.css', adminSession) + expect(deleteThemeAssets).toHaveBeenCalledWith(Number(themeId), ['assets/base.css'], adminSession) expect(renderError).toHaveBeenCalledWith({ headline: 'Failed to delete file "assets/base.css" from remote theme.', body: expect.any(String), diff --git a/packages/theme/src/cli/utilities/theme-fs.ts b/packages/theme/src/cli/utilities/theme-fs.ts index 1b6b211f0e7..cca7c9dfd63 100644 --- a/packages/theme/src/cli/utilities/theme-fs.ts +++ b/packages/theme/src/cli/utilities/theme-fs.ts @@ -9,7 +9,7 @@ import {lookupMimeType, setMimeTypes} from '@shopify/cli-kit/node/mimes' import {outputContent, outputDebug, outputInfo, outputToken, outputWarn} from '@shopify/cli-kit/node/output' 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 {bulkUploadThemeAssets, deleteThemeAssets} from '@shopify/cli-kit/node/themes/api' import EventEmitter from 'node:events' import type { ThemeFileSystem, @@ -191,9 +191,9 @@ export function mountThemeFileSystem(root: string, options?: ThemeFileSystemOpti const syncPromise = options?.noDelete ? Promise.resolve() - : deleteThemeAsset(Number(themeId), fileKey, adminSession) - .then(async (success) => { - if (!success) throw new Error(`Failed to delete file "${fileKey}" from remote theme.`) + : deleteThemeAssets(Number(themeId), [fileKey], adminSession) + .then(async (results) => { + if (!results[0]?.success) throw new Error(`Failed to delete file "${fileKey}" from remote theme.`) unsyncedFileKeys.delete(fileKey) outputSyncResult('delete', fileKey) return true diff --git a/packages/theme/src/cli/utilities/theme-uploader.test.ts b/packages/theme/src/cli/utilities/theme-uploader.test.ts index e7938b58cc8..d99df897810 100644 --- a/packages/theme/src/cli/utilities/theme-uploader.test.ts +++ b/packages/theme/src/cli/utilities/theme-uploader.test.ts @@ -6,7 +6,7 @@ import { uploadTheme, } from './theme-uploader.js' import {fakeThemeFileSystem} from './theme-fs/theme-fs-mock-factory.js' -import {bulkUploadThemeAssets, deleteThemeAsset} from '@shopify/cli-kit/node/themes/api' +import {bulkUploadThemeAssets, deleteThemeAssets} from '@shopify/cli-kit/node/themes/api' import {Result, Checksum, Key, ThemeAsset, Operation} from '@shopify/cli-kit/node/themes/types' import {beforeEach, describe, expect, test, vi} from 'vitest' import {AdminSession} from '@shopify/cli-kit/node/session' @@ -14,7 +14,7 @@ import {AdminSession} from '@shopify/cli-kit/node/session' vi.mock('@shopify/cli-kit/node/themes/api') beforeEach(() => { - vi.mocked(deleteThemeAsset).mockImplementation(() => Promise.resolve(true)) + vi.mocked(deleteThemeAssets).mockImplementation(() => Promise.resolve([])) vi.mocked(bulkUploadThemeAssets).mockImplementation( async ( @@ -65,8 +65,8 @@ describe('theme-uploader', () => { await renderThemeSyncProgress() // Then - expect(vi.mocked(deleteThemeAsset)).toHaveBeenCalledOnce() - expect(vi.mocked(deleteThemeAsset)).toHaveBeenCalledWith(remoteTheme.id, 'assets/deleteme.liquid', adminSession) + expect(vi.mocked(deleteThemeAssets)).toHaveBeenCalledOnce() + expect(vi.mocked(deleteThemeAssets)).toHaveBeenCalledWith(remoteTheme.id, ['assets/deleteme.liquid'], adminSession) }) test('should not delete generated assets', async () => { @@ -86,8 +86,8 @@ describe('theme-uploader', () => { await renderThemeSyncProgress() // Then - expect(vi.mocked(deleteThemeAsset)).toHaveBeenCalledOnce() - expect(vi.mocked(deleteThemeAsset)).toHaveBeenCalledWith(remoteTheme.id, 'assets/base.css.liquid', adminSession) + expect(vi.mocked(deleteThemeAssets)).toHaveBeenCalledOnce() + expect(vi.mocked(deleteThemeAssets)).toHaveBeenCalledWith(remoteTheme.id, ['assets/base.css.liquid'], adminSession) }) test('should not delete files if nodelete is set', async () => { @@ -110,7 +110,7 @@ describe('theme-uploader', () => { await renderThemeSyncProgress() // Then - expect(vi.mocked(deleteThemeAsset)).not.toHaveBeenCalled() + expect(vi.mocked(deleteThemeAssets)).not.toHaveBeenCalled() }) test("should upload a minimum set of files if a theme doesn't exist yet", async () => { @@ -266,19 +266,20 @@ describe('theme-uploader', () => { await renderThemeSyncProgress() // Then - expect(deleteThemeAsset).toHaveBeenCalledTimes(7) - expect(deleteThemeAsset).toHaveBeenNthCalledWith( + expect(deleteThemeAssets).toHaveBeenCalledTimes(1) + expect(deleteThemeAssets).toHaveBeenCalledWith( 1, - remoteTheme.id, - 'templates/product.context.uk.json', + [ + 'templates/product.context.uk.json', + 'templates/product.json', + 'sections/header-group.json', + 'templates/index.liquid', + 'assets/liquid.liquid', + 'config/settings_data.json', + 'assets/image.png', + ], adminSession, ) - expect(deleteThemeAsset).toHaveBeenNthCalledWith(2, remoteTheme.id, 'templates/product.json', adminSession) - expect(deleteThemeAsset).toHaveBeenNthCalledWith(3, remoteTheme.id, 'sections/header-group.json', adminSession) - expect(deleteThemeAsset).toHaveBeenNthCalledWith(4, remoteTheme.id, 'templates/index.liquid', adminSession) - expect(deleteThemeAsset).toHaveBeenNthCalledWith(5, remoteTheme.id, 'assets/liquid.liquid', adminSession) - expect(deleteThemeAsset).toHaveBeenNthCalledWith(6, remoteTheme.id, 'config/settings_data.json', adminSession) - expect(deleteThemeAsset).toHaveBeenNthCalledWith(7, remoteTheme.id, 'assets/image.png', adminSession) }) test('should separate files by type and upload in correct order', async () => { @@ -547,7 +548,7 @@ describe('theme-uploader', () => { await renderThemeSyncProgress() // Then - expect(vi.mocked(deleteThemeAsset)).not.toHaveBeenCalled() + expect(vi.mocked(deleteThemeAssets)).not.toHaveBeenCalled() expect(bulkUploadThemeAssets).toHaveBeenCalledTimes(2) expect(bulkUploadThemeAssets).toHaveBeenCalledWith( remoteTheme.id, @@ -580,8 +581,8 @@ describe('theme-uploader', () => { await renderThemeSyncProgress() // Then - expect(vi.mocked(deleteThemeAsset)).toHaveBeenCalledOnce() - expect(vi.mocked(deleteThemeAsset)).toHaveBeenCalledWith(remoteTheme.id, 'assets/deleteme.liquid', adminSession) + expect(vi.mocked(deleteThemeAssets)).toHaveBeenCalledOnce() + expect(vi.mocked(deleteThemeAssets)).toHaveBeenCalledWith(remoteTheme.id, ['assets/deleteme.liquid'], adminSession) expect(bulkUploadThemeAssets).toHaveBeenCalledTimes(2) expect(bulkUploadThemeAssets).toHaveBeenCalledWith( remoteTheme.id, diff --git a/packages/theme/src/cli/utilities/theme-uploader.ts b/packages/theme/src/cli/utilities/theme-uploader.ts index c5370c5d246..ef64bbd6408 100644 --- a/packages/theme/src/cli/utilities/theme-uploader.ts +++ b/packages/theme/src/cli/utilities/theme-uploader.ts @@ -4,7 +4,7 @@ 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 {AssetParams, bulkUploadThemeAssets, deleteThemeAssets} from '@shopify/cli-kit/node/themes/api' import {Task} from '@shopify/cli-kit/node/ui' import {outputDebug, outputInfo, outputNewline, outputWarn} from '@shopify/cli-kit/node/output' @@ -44,7 +44,7 @@ export function uploadTheme( const deleteJobPromise = uploadJobPromise .then((result) => result.promise) .then(() => reportFailedUploads(uploadResults)) - .then(() => buildDeleteJob(remoteChecksums, themeFileSystem, theme, session, options)) + .then(() => buildDeleteJob(remoteChecksums, themeFileSystem, theme, session, options, uploadResults)) const workPromise = options?.deferPartialWork ? themeCreationPromise @@ -123,6 +123,7 @@ function buildDeleteJob( theme: Theme, session: AdminSession, options: Pick, + uploadResults: Map, ): SyncJob { if (options.nodelete) { return {progress: {current: 0, total: 0}, promise: Promise.resolve()} @@ -131,21 +132,32 @@ 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: Error) => { - failedDeleteAttempts++ - if (failedDeleteAttempts > 3) throw error - createSyncingCatchError(file.key, 'delete')(error) - }) - .finally(() => { - progress.current++ - }), - ), - ).then(() => { + if (orderedFiles.length === 0) { + return {progress, promise: Promise.resolve()} + } + + const deleteBatches = [] + for (let i = 0; i < orderedFiles.length; i += MAX_BATCH_FILE_COUNT) { + const batch = orderedFiles.slice(i, i + MAX_BATCH_FILE_COUNT) + const promise = deleteThemeAssets( + theme.id, + batch.map((file) => file.key), + session, + ).then((results) => { + results.forEach((result) => { + uploadResults.set(result.key, result) + if (!result.success) { + const errorMessage = result.errors?.asset?.map((err) => `-${err}`).join('\n') + createSyncingCatchError(result.key, 'delete')(new Error(`Failed to delete ${result.key}: ${errorMessage}`)) + } + progress.current += batch.length + }) + }) + deleteBatches.push(promise) + } + + const promise = Promise.all(deleteBatches).then(() => { progress.current = progress.total })