Merge pull request #4662 from Shopify/change-nodelete-on-dev

Fix --nodelete behaviour on dev command
Shopify · Oct 25, 2024 · 553a6fb · 553a6fb
2 parents 4436510 + 57bb933
commit 553a6fb
Show file tree

Hide file tree

Showing 5 changed files with 53 additions and 11 deletions.
diff --git a/.changeset/strong-guests-worry.md b/.changeset/strong-guests-worry.md
@@ -0,0 +1,5 @@
+---
+'@shopify/cli': patch
+---
+
+Fix theme dev command deleting remote files even if using --nodelete flag
diff --git a/packages/cli-kit/src/public/node/themes/types.ts b/packages/cli-kit/src/public/node/themes/types.ts
@@ -26,6 +26,7 @@ export type ThemeFSEventPayload<T extends ThemeFSEventName = 'add'> = (ThemeFSEv
 export interface ThemeFileSystemOptions {
   filters?: {ignore?: string[]; only?: string[]}
   notify?: string
+  noDelete?: boolean
 }
 
 /**

diff --git a/packages/theme/src/cli/services/dev.ts b/packages/theme/src/cli/services/dev.ts
@@ -49,6 +49,7 @@ export async function dev(options: DevOptions) {
   const localThemeFileSystem = mountThemeFileSystem(options.directory, {
     filters: options,
     notify: options.notify,
+    noDelete: options.noDelete,
   })
 
   const host = options.host ?? DEFAULT_HOST

diff --git a/packages/theme/src/cli/utilities/theme-fs.test.ts b/packages/theme/src/cli/utilities/theme-fs.test.ts
@@ -529,6 +529,39 @@ describe('theme-fs', () => {
       expect(deleteThemeAsset).toHaveBeenCalledWith(Number(themeId), 'assets/base.css', adminSession)
     })
 
+    test('does not delete file from remote when options.noDelete is true', async () => {
+      // Given
+      vi.mocked(fetchThemeAsset).mockResolvedValue({
+        key: 'assets/base.css',
+        checksum: '1',
+        value: 'content',
+        attachment: '',
+        stats: {size: 100, mtime: 100},
+      })
+      vi.mocked(deleteThemeAsset).mockResolvedValue(true)
+
+      // When
+      const themeFileSystem = mountThemeFileSystem(root, {noDelete: true})
+      await themeFileSystem.ready()
+
+      const deleteOperationPromise = new Promise<void>((resolve) => {
+        themeFileSystem.addEventListener('unlink', () => {
+          setImmediate(resolve)
+        })
+      })
+
+      await themeFileSystem.startWatcher(themeId, adminSession)
+
+      // Explicitly emit the 'unlink' event
+      const watcher = chokidar.watch('') as EventEmitter
+      watcher.emit('unlink', `${root}/assets/base.css`)
+
+      await deleteOperationPromise
+
+      // Then
+      expect(deleteThemeAsset).not.toHaveBeenCalled()
+    })
+
     test('renders a warning to debug if the file deletion fails', async () => {
       // Given
       vi.mocked(fetchThemeAsset).mockResolvedValue({

diff --git a/packages/theme/src/cli/utilities/theme-fs.ts b/packages/theme/src/cli/utilities/theme-fs.ts
@@ -189,17 +189,19 @@ export function mountThemeFileSystem(root: string, options?: ThemeFileSystemOpti
     files.delete(fileKey)
     unsyncedFileKeys.add(fileKey)
 
-    const syncPromise = deleteThemeAsset(Number(themeId), fileKey, adminSession)
-      .then(async (success) => {
-        if (!success) throw new Error(`Failed to delete file "${fileKey}" from remote theme.`)
-        unsyncedFileKeys.delete(fileKey)
-        outputSyncResult('delete', fileKey)
-        return true
-      })
-      .catch((error) => {
-        createSyncingCatchError(fileKey, 'delete')(error)
-        return false
-      })
+    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.`)
+            unsyncedFileKeys.delete(fileKey)
+            outputSyncResult('delete', fileKey)
+            return true
+          })
+          .catch((error) => {
+            createSyncingCatchError(fileKey, 'delete')(error)
+            return false
+          })
 
     emitEvent('unlink', {
       fileKey,