From ca07b64c49409bdcabcc71eedc7340190367b49e Mon Sep 17 00:00:00 2001 From: James Meng Date: Wed, 28 Aug 2024 15:45:50 -0700 Subject: [PATCH 1/6] Fix Hanging Theme Reconciliation Progress Bar --- .../remote-theme-watcher.test.ts | 5 +- .../theme-environment/remote-theme-watcher.ts | 18 ++-- .../theme-environment.test.ts | 5 ++ .../theme-environment/theme-environment.ts | 69 +++++++++++--- .../theme-reconciliation.test.ts | 89 +++++++++++++++---- .../theme-environment/theme-reconciliation.ts | 21 ++++- 6 files changed, 168 insertions(+), 39 deletions(-) diff --git a/packages/theme/src/cli/utilities/theme-environment/remote-theme-watcher.test.ts b/packages/theme/src/cli/utilities/theme-environment/remote-theme-watcher.test.ts index 1a75e6b93c..4ad1245468 100644 --- a/packages/theme/src/cli/utilities/theme-environment/remote-theme-watcher.test.ts +++ b/packages/theme/src/cli/utilities/theme-environment/remote-theme-watcher.test.ts @@ -23,7 +23,10 @@ describe('reconcileAndPollThemeEditorChanges', async () => { const defaultThemeFileSystem = fakeThemeFileSystem('tmp', files) const initialRemoteChecksums = [{checksum: '1', key: 'templates/asset.json'}] - vi.mocked(reconcileJsonFiles).mockResolvedValue(undefined) + vi.mocked(reconcileJsonFiles).mockResolvedValue({ + reconciliationFinishedPromise: Promise.resolve(), + readyForReconciliationPromise: Promise.resolve(), + }) vi.mocked(fetchChecksums).mockResolvedValue([{checksum: '2', key: 'templates/asset.json'}]) // When diff --git a/packages/theme/src/cli/utilities/theme-environment/remote-theme-watcher.ts b/packages/theme/src/cli/utilities/theme-environment/remote-theme-watcher.ts index 1e47343b93..8055203578 100644 --- a/packages/theme/src/cli/utilities/theme-environment/remote-theme-watcher.ts +++ b/packages/theme/src/cli/utilities/theme-environment/remote-theme-watcher.ts @@ -22,16 +22,24 @@ export async function reconcileAndPollThemeEditorChanges( ignore: string[] only: string[] }, -) { +): Promise<{ + updatedRemoteChecksums: Checksum[] + reconciliationFinishedPromise: Promise + readyForReconciliationPromise: Promise +}> { outputDebug('Initiating theme asset reconciliation process') await localThemeFileSystem.ready() - if (remoteChecksums.length !== 0) { - await reconcileJsonFiles(targetTheme, session, remoteChecksums, localThemeFileSystem, options) - } + const {reconciliationFinishedPromise, readyForReconciliationPromise} = await reconcileJsonFiles( + targetTheme, + session, + remoteChecksums, + localThemeFileSystem, + options, + ) const updatedRemoteChecksums = await fetchChecksums(targetTheme.id, session) pollThemeEditorChanges(targetTheme, session, updatedRemoteChecksums, localThemeFileSystem, options) - return updatedRemoteChecksums + return {updatedRemoteChecksums, reconciliationFinishedPromise, readyForReconciliationPromise} } diff --git a/packages/theme/src/cli/utilities/theme-environment/theme-environment.test.ts b/packages/theme/src/cli/utilities/theme-environment/theme-environment.test.ts index 773e20968d..8a381510ff 100644 --- a/packages/theme/src/cli/utilities/theme-environment/theme-environment.test.ts +++ b/packages/theme/src/cli/utilities/theme-environment/theme-environment.test.ts @@ -104,6 +104,11 @@ describe('setupDevServer', () => { ...filters, }, } + vi.mocked(reconcileAndPollThemeEditorChanges).mockResolvedValue({ + updatedRemoteChecksums: [], + readyForReconciliationPromise: Promise.resolve(), + reconciliationFinishedPromise: Promise.resolve(), + }) // When await setupDevServer(developmentTheme, context).workPromise diff --git a/packages/theme/src/cli/utilities/theme-environment/theme-environment.ts b/packages/theme/src/cli/utilities/theme-environment/theme-environment.ts index e1c9f7f891..53d41b1ce2 100644 --- a/packages/theme/src/cli/utilities/theme-environment/theme-environment.ts +++ b/packages/theme/src/cli/utilities/theme-environment/theme-environment.ts @@ -4,6 +4,7 @@ import {getHtmlHandler} from './html.js' import {getAssetsHandler} from './local-assets.js' import {getProxyHandler} from './proxy.js' import {uploadTheme} from '../theme-uploader.js' +import {renderTasksToStdErr} from '../theme-ui.js' import {createApp, defineEventHandler, defineLazyEventHandler, toNodeListener} from 'h3' import {fetchChecksums} from '@shopify/cli-kit/node/themes/api' import {createServer} from 'node:http' @@ -27,33 +28,73 @@ export function setupDevServer(theme: Theme, ctx: DevServerContext) { function ensureThemeEnvironmentSetup(theme: Theme, ctx: DevServerContext) { const remoteChecksumsPromise = fetchChecksums(theme.id, ctx.session) - const reconcilePromise = remoteChecksumsPromise.then((remoteChecksums) => - ctx.options.themeEditorSync - ? reconcileAndPollThemeEditorChanges(theme, ctx.session, remoteChecksums, ctx.localThemeFileSystem, { - noDelete: ctx.options.noDelete, + const editorSyncPromise = remoteChecksumsPromise.then((remoteChecksums) => + handleThemeEditorSync(theme, ctx, remoteChecksums), + ) + + const themeSetupPromise = editorSyncPromise.then( + ({updatedRemoteChecksums, reconciliationFinishedPromise, readyForReconciliationPromise}) => { + const uploadPromise = reconciliationFinishedPromise.then(() => + uploadTheme(theme, ctx.session, updatedRemoteChecksums, ctx.localThemeFileSystem, { + nodelete: ctx.options.noDelete, ignore: ctx.options.ignore, only: ctx.options.only, - }) - : remoteChecksums, - ) + deferPartialWork: true, + }), + ) - const uploadPromise = reconcilePromise.then(async (remoteChecksums: Checksum[]) => - uploadTheme(theme, ctx.session, remoteChecksums, ctx.localThemeFileSystem, { - nodelete: ctx.options.noDelete, - deferPartialWork: true, - }), + return {uploadPromise, reconciliationFinishedPromise, readyForReconciliationPromise} + }, ) return { - workPromise: uploadPromise.then((result) => result.workPromise), + workPromise: themeSetupPromise.then(async ({uploadPromise}) => (await uploadPromise).workPromise), renderProgress: async () => { - const {renderThemeSyncProgress} = await uploadPromise + if (ctx.options.themeEditorSync) { + await themeSetupPromise.then(({readyForReconciliationPromise}) => readyForReconciliationPromise) + await renderTasksToStdErr([ + { + title: 'Performing file synchronization. This may take a while...', + task: async () => { + await themeSetupPromise.then(async ({reconciliationFinishedPromise}) => { + await reconciliationFinishedPromise + }) + }, + }, + ]) + } + + const {renderThemeSyncProgress} = await themeSetupPromise.then(({uploadPromise}) => uploadPromise) await renderThemeSyncProgress() }, } } +function handleThemeEditorSync( + theme: Theme, + ctx: DevServerContext, + remoteChecksums: Checksum[], +): Promise<{ + updatedRemoteChecksums: Checksum[] + reconciliationFinishedPromise: Promise + readyForReconciliationPromise: Promise +}> { + if (ctx.options.themeEditorSync) { + return reconcileAndPollThemeEditorChanges(theme, ctx.session, remoteChecksums, ctx.localThemeFileSystem, { + noDelete: ctx.options.noDelete, + ignore: ctx.options.ignore, + only: ctx.options.only, + }) + } else { + return Promise.resolve({ + updatedRemoteChecksums: remoteChecksums, + reconciliationFinishedPromise: Promise.resolve(), + readyForReconciliationPromise: Promise.resolve(), + }) + } +} + interface DevelopmentServerInstance { close: () => Promise } 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 ae908c8f87..020ab297e8 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 @@ -33,7 +33,13 @@ describe('reconcileThemeFiles', () => { ] // When - await reconcileJsonFiles(developmentTheme, adminSession, remoteChecksums, defaultThemeFileSystem, defaultOptions) + await reconcileAndWaitForReconciliationFinish( + developmentTheme, + adminSession, + remoteChecksums, + defaultThemeFileSystem, + defaultOptions, + ) // Then expect(fetchThemeAsset).toHaveBeenCalledTimes(1) @@ -51,10 +57,16 @@ describe('reconcileThemeFiles', () => { ] // When - await reconcileJsonFiles(developmentTheme, adminSession, remoteChecksums, defaultThemeFileSystem, { - ...defaultOptions, - only: ['templates/*'], - }) + await reconcileAndWaitForReconciliationFinish( + developmentTheme, + adminSession, + remoteChecksums, + defaultThemeFileSystem, + { + ...defaultOptions, + only: ['templates/*'], + }, + ) // Then expect(fetchThemeAsset).toHaveBeenCalledTimes(1) @@ -93,7 +105,13 @@ describe('reconcileThemeFiles', () => { // When expect(defaultThemeFileSystem.files.get('templates/asset.json')).toBeUndefined() - await reconcileJsonFiles(developmentTheme, adminSession, remoteChecksums, defaultThemeFileSystem, defaultOptions) + await reconcileAndWaitForReconciliationFinish( + developmentTheme, + adminSession, + remoteChecksums, + defaultThemeFileSystem, + defaultOptions, + ) // Then expect(fetchThemeAsset).toHaveBeenCalledWith(developmentTheme.id, assetToBeDownloaded.key, adminSession) @@ -107,7 +125,13 @@ describe('reconcileThemeFiles', () => { const remoteChecksums = [assetToBeDeleted] // When - await reconcileJsonFiles(developmentTheme, adminSession, remoteChecksums, defaultThemeFileSystem, defaultOptions) + await reconcileAndWaitForReconciliationFinish( + developmentTheme, + adminSession, + remoteChecksums, + defaultThemeFileSystem, + defaultOptions, + ) // Then expect(deleteThemeAsset).toHaveBeenCalledWith(developmentTheme.id, assetToBeDeleted.key, adminSession) @@ -124,7 +148,13 @@ describe('reconcileThemeFiles', () => { const spy = vi.spyOn(localThemeFileSystem, 'delete') // When - await reconcileJsonFiles(developmentTheme, adminSession, remoteChecksums, localThemeFileSystem, defaultOptions) + await reconcileAndWaitForReconciliationFinish( + developmentTheme, + adminSession, + remoteChecksums, + localThemeFileSystem, + defaultOptions, + ) // Then expect(spy).toHaveBeenCalledWith('templates/asset.json') @@ -138,7 +168,13 @@ describe('reconcileThemeFiles', () => { const spy = vi.spyOn(localThemeFileSystem, 'delete') // When - await reconcileJsonFiles(developmentTheme, adminSession, remoteChecksums, localThemeFileSystem, defaultOptions) + await reconcileAndWaitForReconciliationFinish( + developmentTheme, + adminSession, + remoteChecksums, + localThemeFileSystem, + defaultOptions, + ) // Then expect(spy).not.toHaveBeenCalled() @@ -151,10 +187,16 @@ describe('reconcileThemeFiles', () => { const spy = vi.spyOn(localThemeFileSystem, 'delete') // When - await reconcileJsonFiles(developmentTheme, adminSession, remoteChecksums, localThemeFileSystem, { - ...defaultOptions, - noDelete: true, - }) + await reconcileAndWaitForReconciliationFinish( + developmentTheme, + adminSession, + remoteChecksums, + localThemeFileSystem, + { + ...defaultOptions, + noDelete: true, + }, + ) // Then expect(renderSelectPrompt).not.toHaveBeenCalled() @@ -171,7 +213,13 @@ describe('reconcileThemeFiles', () => { const remoteChecksums = [{checksum: '2', key: 'templates/asset.json'}] // When - await reconcileJsonFiles(developmentTheme, adminSession, remoteChecksums, localThemeFileSystem, defaultOptions) + await reconcileAndWaitForReconciliationFinish( + developmentTheme, + adminSession, + remoteChecksums, + localThemeFileSystem, + defaultOptions, + ) // Then expect(fetchThemeAsset).toHaveBeenCalled() @@ -185,10 +233,21 @@ describe('reconcileThemeFiles', () => { const remoteChecksums = [{checksum: '2', key: 'templates/asset.json'}] // When - await reconcileJsonFiles(developmentTheme, adminSession, remoteChecksums, localThemeFileSystem, defaultOptions) + await reconcileAndWaitForReconciliationFinish( + developmentTheme, + adminSession, + remoteChecksums, + localThemeFileSystem, + defaultOptions, + ) // Then expect(fetchThemeAsset).not.toHaveBeenCalled() }) }) }) + +async function reconcileAndWaitForReconciliationFinish(...args: Parameters) { + const {reconciliationFinishedPromise} = await reconcileJsonFiles(...args) + return reconciliationFinishedPromise +} 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 65f03175aa..7f2c65ab3d 100644 --- a/packages/theme/src/cli/utilities/theme-environment/theme-reconciliation.ts +++ b/packages/theme/src/cli/utilities/theme-environment/theme-reconciliation.ts @@ -25,7 +25,10 @@ export async function reconcileJsonFiles( remoteChecksums: Checksum[], localThemeFileSystem: ThemeFileSystem, options: ReconciliationOptions, -) { +): Promise<{ + readyForReconciliationPromise: Promise + reconciliationFinishedPromise: Promise +}> { outputDebug('Initiating theme asset reconciliation process') const {filesOnlyPresentLocally, filesOnlyPresentOnRemote, filesWithConflictingChecksums} = identifyFilesToReconcile( @@ -39,10 +42,13 @@ export async function reconcileJsonFiles( filesWithConflictingChecksums.length === 0 ) { outputDebug('Local and remote checksums match - no need to reconcile theme assets') - return localThemeFileSystem + return { + readyForReconciliationPromise: Promise.resolve(), + reconciliationFinishedPromise: Promise.resolve(), + } } - const partitionedFiles = await partitionFilesByReconciliationStrategy( + const readyForReconciliationPromise = partitionFilesByReconciliationStrategy( { filesOnlyPresentLocally, filesOnlyPresentOnRemote, @@ -51,7 +57,14 @@ export async function reconcileJsonFiles( options, ) - await performFileReconciliation(targetTheme, session, localThemeFileSystem, partitionedFiles) + const reconciliationFinishedPromise = readyForReconciliationPromise.then((partitionedFiles) => + performFileReconciliation(targetTheme, session, localThemeFileSystem, partitionedFiles), + ) + + return { + readyForReconciliationPromise: readyForReconciliationPromise.then(() => {}), + reconciliationFinishedPromise, + } } function identifyFilesToReconcile( From 4ea6d5d3cf038768dd0221ee41aac3a26b14351a Mon Sep 17 00:00:00 2001 From: James Meng Date: Fri, 30 Aug 2024 15:55:33 -0700 Subject: [PATCH 2/6] Separate theme editor sync prompting process from worker process --- .../theme-environment/remote-theme-watcher.ts | 22 +++++--- .../theme-environment/theme-environment.ts | 56 +++++++------------ .../theme-environment/theme-reconciliation.ts | 36 ++++++------ 3 files changed, 52 insertions(+), 62 deletions(-) diff --git a/packages/theme/src/cli/utilities/theme-environment/remote-theme-watcher.ts b/packages/theme/src/cli/utilities/theme-environment/remote-theme-watcher.ts index 8055203578..fb56e9c921 100644 --- a/packages/theme/src/cli/utilities/theme-environment/remote-theme-watcher.ts +++ b/packages/theme/src/cli/utilities/theme-environment/remote-theme-watcher.ts @@ -1,5 +1,6 @@ import {pollThemeEditorChanges} from './theme-polling.js' import {reconcileJsonFiles} from './theme-reconciliation.js' +import {outputDebug} from '@shopify/cli-kit/node/output' import {AdminSession} from '@shopify/cli-kit/node/session' import {Checksum, Theme, ThemeFileSystem} from '@shopify/cli-kit/node/themes/types' import {fetchChecksums} from '@shopify/cli-kit/node/themes/api' @@ -23,23 +24,26 @@ export async function reconcileAndPollThemeEditorChanges( only: string[] }, ): Promise<{ - updatedRemoteChecksums: Checksum[] - reconciliationFinishedPromise: Promise - readyForReconciliationPromise: Promise + updatedRemoteChecksumsPromise: Promise + userInputPromise: Promise + workPromise: Promise }> { outputDebug('Initiating theme asset reconciliation process') await localThemeFileSystem.ready() - const {reconciliationFinishedPromise, readyForReconciliationPromise} = await reconcileJsonFiles( - targetTheme, - session, + const {userInputPromise, workPromise} = await reconcileJsonFiles( remoteChecksums, localThemeFileSystem, + targetTheme, + session, options, ) - const updatedRemoteChecksums = await fetchChecksums(targetTheme.id, session) - pollThemeEditorChanges(targetTheme, session, updatedRemoteChecksums, localThemeFileSystem, options) + const updatedRemoteChecksumsPromise = workPromise.then(async () => { + const updatedRemoteChecksums = await fetchChecksums(targetTheme.id, session) + pollThemeEditorChanges(targetTheme, session, updatedRemoteChecksums, localThemeFileSystem, options) + return updatedRemoteChecksums + }) - return {updatedRemoteChecksums, reconciliationFinishedPromise, readyForReconciliationPromise} + return {updatedRemoteChecksumsPromise, userInputPromise, workPromise} } diff --git a/packages/theme/src/cli/utilities/theme-environment/theme-environment.ts b/packages/theme/src/cli/utilities/theme-environment/theme-environment.ts index 53d41b1ce2..f70d61fa61 100644 --- a/packages/theme/src/cli/utilities/theme-environment/theme-environment.ts +++ b/packages/theme/src/cli/utilities/theme-environment/theme-environment.ts @@ -1,8 +1,8 @@ -import {reconcileAndPollThemeEditorChanges} from './remote-theme-watcher.js' import {getHotReloadHandler, setupInMemoryTemplateWatcher} from './hot-reload/server.js' import {getHtmlHandler} from './html.js' import {getAssetsHandler} from './local-assets.js' 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 {createApp, defineEventHandler, defineLazyEventHandler, toNodeListener} from 'h3' @@ -28,58 +28,42 @@ export function setupDevServer(theme: Theme, ctx: DevServerContext) { function ensureThemeEnvironmentSetup(theme: Theme, ctx: DevServerContext) { const remoteChecksumsPromise = fetchChecksums(theme.id, ctx.session) - const editorSyncPromise = remoteChecksumsPromise.then((remoteChecksums) => + const reconcilePromise = remoteChecksumsPromise.then((remoteChecksums) => handleThemeEditorSync(theme, ctx, remoteChecksums), ) - const themeSetupPromise = editorSyncPromise.then( - ({updatedRemoteChecksums, reconciliationFinishedPromise, readyForReconciliationPromise}) => { - const uploadPromise = reconciliationFinishedPromise.then(() => - uploadTheme(theme, ctx.session, updatedRemoteChecksums, ctx.localThemeFileSystem, { - nodelete: ctx.options.noDelete, - ignore: ctx.options.ignore, - only: ctx.options.only, - deferPartialWork: true, - }), - ) - - return {uploadPromise, reconciliationFinishedPromise, readyForReconciliationPromise} - }, - ) + const uploadPromise = reconcilePromise.then(async ({updatedRemoteChecksumsPromise}) => { + const updatedRemoteChecksums = await updatedRemoteChecksumsPromise + return uploadTheme(theme, ctx.session, updatedRemoteChecksums, ctx.localThemeFileSystem, { + nodelete: ctx.options.noDelete, + deferPartialWork: true, + }) + }) return { - workPromise: themeSetupPromise.then(async ({uploadPromise}) => (await uploadPromise).workPromise), + workPromise: uploadPromise.then((result) => result.workPromise), renderProgress: async () => { if (ctx.options.themeEditorSync) { - await themeSetupPromise.then(({readyForReconciliationPromise}) => readyForReconciliationPromise) + const {userInputPromise, workPromise} = await reconcilePromise + await userInputPromise await renderTasksToStdErr([ { title: 'Performing file synchronization. This may take a while...', task: async () => { - await themeSetupPromise.then(async ({reconciliationFinishedPromise}) => { - await reconciliationFinishedPromise - }) + await workPromise }, }, ]) } - const {renderThemeSyncProgress} = await themeSetupPromise.then(({uploadPromise}) => uploadPromise) + const {renderThemeSyncProgress} = await uploadPromise await renderThemeSyncProgress() }, } } -function handleThemeEditorSync( - theme: Theme, - ctx: DevServerContext, - remoteChecksums: Checksum[], -): Promise<{ - updatedRemoteChecksums: Checksum[] - reconciliationFinishedPromise: Promise - readyForReconciliationPromise: Promise -}> { +function handleThemeEditorSync(theme: Theme, ctx: DevServerContext, remoteChecksums: Checksum[]) { if (ctx.options.themeEditorSync) { return reconcileAndPollThemeEditorChanges(theme, ctx.session, remoteChecksums, ctx.localThemeFileSystem, { noDelete: ctx.options.noDelete, @@ -87,11 +71,11 @@ function handleThemeEditorSync( only: ctx.options.only, }) } else { - return Promise.resolve({ - updatedRemoteChecksums: remoteChecksums, - reconciliationFinishedPromise: Promise.resolve(), - readyForReconciliationPromise: Promise.resolve(), - }) + return { + updatedRemoteChecksumsPromise: Promise.resolve(remoteChecksums), + userInputPromise: Promise.resolve(), + workPromise: Promise.resolve(), + } } } 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 7f2c65ab3d..63ebfa715f 100644 --- a/packages/theme/src/cli/utilities/theme-environment/theme-reconciliation.ts +++ b/packages/theme/src/cli/utilities/theme-environment/theme-reconciliation.ts @@ -19,6 +19,10 @@ interface ReconciliationOptions { ignore: string[] } +const noWorkPromise = { + workPromise: Promise.resolve(), +} + export async function reconcileJsonFiles( targetTheme: Theme, session: AdminSession, @@ -26,15 +30,16 @@ export async function reconcileJsonFiles( localThemeFileSystem: ThemeFileSystem, options: ReconciliationOptions, ): Promise<{ - readyForReconciliationPromise: Promise - reconciliationFinishedPromise: Promise + workPromise: Promise }> { + if (remoteChecksums.length === 0) { + return noWorkPromise + } + outputDebug('Initiating theme asset reconciliation process') - const {filesOnlyPresentLocally, filesOnlyPresentOnRemote, filesWithConflictingChecksums} = identifyFilesToReconcile( - remoteChecksums, - localThemeFileSystem, - ) + const {filesOnlyPresentLocally, filesOnlyPresentOnRemote, filesWithConflictingChecksums} = + await identifyFilesToReconcile(remoteChecksums, localThemeFileSystem) if ( filesOnlyPresentLocally.length === 0 && @@ -42,13 +47,10 @@ export async function reconcileJsonFiles( filesWithConflictingChecksums.length === 0 ) { outputDebug('Local and remote checksums match - no need to reconcile theme assets') - return { - readyForReconciliationPromise: Promise.resolve(), - reconciliationFinishedPromise: Promise.resolve(), - } + return noWorkPromise } - const readyForReconciliationPromise = partitionFilesByReconciliationStrategy( + const partitionedFiles = await partitionFilesByReconciliationStrategy( { filesOnlyPresentLocally, filesOnlyPresentOnRemote, @@ -57,14 +59,14 @@ export async function reconcileJsonFiles( options, ) - const reconciliationFinishedPromise = readyForReconciliationPromise.then((partitionedFiles) => - performFileReconciliation(targetTheme, session, localThemeFileSystem, partitionedFiles), + const fileReconciliationPromise = performFileReconciliation( + targetTheme, + session, + localThemeFileSystem, + partitionedFiles, ) - return { - readyForReconciliationPromise: readyForReconciliationPromise.then(() => {}), - reconciliationFinishedPromise, - } + return {workPromise: fileReconciliationPromise} } function identifyFilesToReconcile( From 294d104873c485f38e0b7f05559c7b5d613c04d1 Mon Sep 17 00:00:00 2001 From: James Meng Date: Fri, 30 Aug 2024 15:17:23 -0700 Subject: [PATCH 3/6] await theme-editor-sync user prompts upfront --- .../theme-environment/remote-theme-watcher.ts | 11 ++--------- .../theme-environment/theme-environment.ts | 17 +++++++++++------ 2 files changed, 13 insertions(+), 15 deletions(-) diff --git a/packages/theme/src/cli/utilities/theme-environment/remote-theme-watcher.ts b/packages/theme/src/cli/utilities/theme-environment/remote-theme-watcher.ts index fb56e9c921..bb04ab8697 100644 --- a/packages/theme/src/cli/utilities/theme-environment/remote-theme-watcher.ts +++ b/packages/theme/src/cli/utilities/theme-environment/remote-theme-watcher.ts @@ -25,19 +25,12 @@ export async function reconcileAndPollThemeEditorChanges( }, ): Promise<{ updatedRemoteChecksumsPromise: Promise - userInputPromise: Promise workPromise: Promise }> { outputDebug('Initiating theme asset reconciliation process') await localThemeFileSystem.ready() - const {userInputPromise, workPromise} = await reconcileJsonFiles( - remoteChecksums, - localThemeFileSystem, - targetTheme, - session, - options, - ) + const {workPromise} = await reconcileJsonFiles(remoteChecksums, localThemeFileSystem, targetTheme, session, options) const updatedRemoteChecksumsPromise = workPromise.then(async () => { const updatedRemoteChecksums = await fetchChecksums(targetTheme.id, session) @@ -45,5 +38,5 @@ export async function reconcileAndPollThemeEditorChanges( return updatedRemoteChecksums }) - return {updatedRemoteChecksumsPromise, userInputPromise, workPromise} + return {updatedRemoteChecksumsPromise, workPromise} } diff --git a/packages/theme/src/cli/utilities/theme-environment/theme-environment.ts b/packages/theme/src/cli/utilities/theme-environment/theme-environment.ts index f70d61fa61..76dcf46f6c 100644 --- a/packages/theme/src/cli/utilities/theme-environment/theme-environment.ts +++ b/packages/theme/src/cli/utilities/theme-environment/theme-environment.ts @@ -44,8 +44,7 @@ function ensureThemeEnvironmentSetup(theme: Theme, ctx: DevServerContext) { workPromise: uploadPromise.then((result) => result.workPromise), renderProgress: async () => { if (ctx.options.themeEditorSync) { - const {userInputPromise, workPromise} = await reconcilePromise - await userInputPromise + const {workPromise} = await reconcilePromise await renderTasksToStdErr([ { title: 'Performing file synchronization. This may take a while...', @@ -63,7 +62,14 @@ function ensureThemeEnvironmentSetup(theme: Theme, ctx: DevServerContext) { } } -function handleThemeEditorSync(theme: Theme, ctx: DevServerContext, remoteChecksums: Checksum[]) { +function handleThemeEditorSync( + theme: Theme, + ctx: DevServerContext, + remoteChecksums: Checksum[], +): Promise<{ + updatedRemoteChecksumsPromise: Promise + workPromise: Promise +}> { if (ctx.options.themeEditorSync) { return reconcileAndPollThemeEditorChanges(theme, ctx.session, remoteChecksums, ctx.localThemeFileSystem, { noDelete: ctx.options.noDelete, @@ -71,11 +77,10 @@ function handleThemeEditorSync(theme: Theme, ctx: DevServerContext, remoteChecks only: ctx.options.only, }) } else { - return { + return Promise.resolve({ updatedRemoteChecksumsPromise: Promise.resolve(remoteChecksums), - userInputPromise: Promise.resolve(), workPromise: Promise.resolve(), - } + }) } } From 876f6a24e164ae4e40608ed8e202d59f4c5b3c66 Mon Sep 17 00:00:00 2001 From: James Meng Date: Fri, 30 Aug 2024 15:17:51 -0700 Subject: [PATCH 4/6] await password prompt prior to dev server setup if theme-editor-sync flag is provided --- packages/theme/src/cli/services/dev.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/theme/src/cli/services/dev.ts b/packages/theme/src/cli/services/dev.ts index a5ed52ce8a..94371a70d8 100644 --- a/packages/theme/src/cli/services/dev.ts +++ b/packages/theme/src/cli/services/dev.ts @@ -95,10 +95,15 @@ export async function dev(options: DevOptions) { }, } + if (options['theme-editor-sync']) { + session.storefrontPassword = await storefrontPasswordPromise + } + const {serverStart, renderDevSetupProgress} = setupDevServer(options.theme, ctx) - const storefrontPassword = await storefrontPasswordPromise - session.storefrontPassword = storefrontPassword + if (!options['theme-editor-sync']) { + session.storefrontPassword = await storefrontPasswordPromise + } await renderDevSetupProgress() await serverStart() From 911b55bf432477f1123de48375ab9661f426e4c2 Mon Sep 17 00:00:00 2001 From: James Meng Date: Fri, 30 Aug 2024 16:18:09 -0700 Subject: [PATCH 5/6] remove options arg --- .../src/cli/utilities/theme-environment/remote-theme-watcher.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/theme/src/cli/utilities/theme-environment/remote-theme-watcher.ts b/packages/theme/src/cli/utilities/theme-environment/remote-theme-watcher.ts index bb04ab8697..ca29525af9 100644 --- a/packages/theme/src/cli/utilities/theme-environment/remote-theme-watcher.ts +++ b/packages/theme/src/cli/utilities/theme-environment/remote-theme-watcher.ts @@ -4,7 +4,6 @@ import {outputDebug} from '@shopify/cli-kit/node/output' import {AdminSession} from '@shopify/cli-kit/node/session' import {Checksum, Theme, ThemeFileSystem} from '@shopify/cli-kit/node/themes/types' import {fetchChecksums} from '@shopify/cli-kit/node/themes/api' -import {outputDebug} from '@shopify/cli-kit/node/output' export const LOCAL_STRATEGY = 'local' export const REMOTE_STRATEGY = 'remote' From 9fb98cee368c53befe1fbf79b88bb81215b77e44 Mon Sep 17 00:00:00 2001 From: James Meng Date: Fri, 30 Aug 2024 17:00:57 -0700 Subject: [PATCH 6/6] update tests --- .../remote-theme-watcher.test.ts | 44 +++++-------------- .../theme-environment/remote-theme-watcher.ts | 2 +- .../theme-environment.test.ts | 5 +-- .../theme-reconciliation.test.ts | 33 +++++++++++--- 4 files changed, 41 insertions(+), 43 deletions(-) diff --git a/packages/theme/src/cli/utilities/theme-environment/remote-theme-watcher.test.ts b/packages/theme/src/cli/utilities/theme-environment/remote-theme-watcher.test.ts index 4ad1245468..f34edcfea6 100644 --- a/packages/theme/src/cli/utilities/theme-environment/remote-theme-watcher.test.ts +++ b/packages/theme/src/cli/utilities/theme-environment/remote-theme-watcher.test.ts @@ -6,7 +6,7 @@ import {fetchChecksums} from '@shopify/cli-kit/node/themes/api' import {buildTheme} from '@shopify/cli-kit/node/themes/factories' import {ThemeAsset} from '@shopify/cli-kit/node/themes/types' import {DEVELOPMENT_THEME_ROLE} from '@shopify/cli-kit/node/themes/utils' -import {describe, expect, test, vi} from 'vitest' +import {beforeEach, describe, expect, test, vi} from 'vitest' vi.mock('@shopify/cli-kit/node/themes/api') vi.mock('./theme-reconciliation.js') @@ -17,20 +17,22 @@ describe('reconcileAndPollThemeEditorChanges', async () => { const developmentTheme = buildTheme({id: 1, name: 'Theme', role: DEVELOPMENT_THEME_ROLE})! const adminSession = {token: '', storeFqdn: ''} + beforeEach(() => { + vi.mocked(reconcileJsonFiles).mockResolvedValue({ + workPromise: Promise.resolve(), + }) + }) + test('should call pollThemeEditorChanges with updated checksums if the remote theme was been updated during reconciliation', async () => { // Given const files = new Map([]) const defaultThemeFileSystem = fakeThemeFileSystem('tmp', files) const initialRemoteChecksums = [{checksum: '1', key: 'templates/asset.json'}] - vi.mocked(reconcileJsonFiles).mockResolvedValue({ - reconciliationFinishedPromise: Promise.resolve(), - readyForReconciliationPromise: Promise.resolve(), - }) vi.mocked(fetchChecksums).mockResolvedValue([{checksum: '2', key: 'templates/asset.json'}]) // When - await reconcileAndPollThemeEditorChanges( + const {workPromise, updatedRemoteChecksumsPromise} = await reconcileAndPollThemeEditorChanges( developmentTheme, adminSession, initialRemoteChecksums, @@ -41,6 +43,8 @@ describe('reconcileAndPollThemeEditorChanges', async () => { only: [], }, ) + await workPromise + await updatedRemoteChecksumsPromise // Then expect(pollThemeEditorChanges).toHaveBeenCalledWith( @@ -56,34 +60,6 @@ describe('reconcileAndPollThemeEditorChanges', async () => { ) }) - test('should not call reconcileJsonFiles when remote theme contains no files', async () => { - // Given - const files = new Map([]) - const defaultThemeFileSystem = fakeThemeFileSystem('tmp', files) - const emptyRemoteChecksums: [] = [] - const newFileSystem = fakeThemeFileSystem('tmp', new Map([])) - - vi.mocked(fetchChecksums).mockResolvedValue([]) - const readySpy = vi.spyOn(defaultThemeFileSystem, 'ready').mockResolvedValue() - - // When - await reconcileAndPollThemeEditorChanges( - developmentTheme, - adminSession, - emptyRemoteChecksums, - defaultThemeFileSystem, - { - noDelete: false, - ignore: [], - only: [], - }, - ) - - // Then - expect(reconcileJsonFiles).not.toHaveBeenCalled() - expect(pollThemeEditorChanges).toHaveBeenCalled() - }) - test('should wait for the local theme file system to be ready before reconciling', async () => { // Given const files = new Map([]) diff --git a/packages/theme/src/cli/utilities/theme-environment/remote-theme-watcher.ts b/packages/theme/src/cli/utilities/theme-environment/remote-theme-watcher.ts index ca29525af9..a8d7470b73 100644 --- a/packages/theme/src/cli/utilities/theme-environment/remote-theme-watcher.ts +++ b/packages/theme/src/cli/utilities/theme-environment/remote-theme-watcher.ts @@ -29,7 +29,7 @@ export async function reconcileAndPollThemeEditorChanges( outputDebug('Initiating theme asset reconciliation process') await localThemeFileSystem.ready() - const {workPromise} = await reconcileJsonFiles(remoteChecksums, localThemeFileSystem, targetTheme, session, options) + const {workPromise} = await reconcileJsonFiles(targetTheme, session, remoteChecksums, localThemeFileSystem, options) const updatedRemoteChecksumsPromise = workPromise.then(async () => { const updatedRemoteChecksums = await fetchChecksums(targetTheme.id, session) diff --git a/packages/theme/src/cli/utilities/theme-environment/theme-environment.test.ts b/packages/theme/src/cli/utilities/theme-environment/theme-environment.test.ts index 8a381510ff..497f15bd27 100644 --- a/packages/theme/src/cli/utilities/theme-environment/theme-environment.test.ts +++ b/packages/theme/src/cli/utilities/theme-environment/theme-environment.test.ts @@ -105,9 +105,8 @@ describe('setupDevServer', () => { }, } vi.mocked(reconcileAndPollThemeEditorChanges).mockResolvedValue({ - updatedRemoteChecksums: [], - readyForReconciliationPromise: Promise.resolve(), - reconciliationFinishedPromise: Promise.resolve(), + updatedRemoteChecksumsPromise: Promise.resolve([]), + workPromise: Promise.resolve(), }) // When 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 020ab297e8..c6ddcf0704 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 @@ -14,10 +14,10 @@ vi.mock('./theme-fs.js') vi.mock('./theme-downloader.js') vi.mock('./theme-uploader.js') -describe('reconcileThemeFiles', () => { +describe('reconcileJsonFiles', () => { const developmentTheme = buildTheme({id: 1, name: 'Theme', role: DEVELOPMENT_THEME_ROLE})! const adminSession = {token: '', storeFqdn: ''} - const remoteChecksums: Checksum[] = [] + const remoteChecksums: Checksum[] = [{checksum: '1', key: 'config/settings_schema.json'}] const files = new Map([]) const defaultThemeFileSystem = fakeThemeFileSystem('tmp', files) const defaultOptions = {noDelete: false, only: [], ignore: []} @@ -199,7 +199,11 @@ describe('reconcileThemeFiles', () => { ) // Then - expect(renderSelectPrompt).not.toHaveBeenCalled() + expect(renderSelectPrompt).not.toHaveBeenCalledWith( + expect.any(Array), + 'The files listed below are only present locally. What would you like to do?', + expect.any(Object), + ) expect(spy).not.toHaveBeenCalledWith() }) }) @@ -245,9 +249,28 @@ describe('reconcileThemeFiles', () => { expect(fetchThemeAsset).not.toHaveBeenCalled() }) }) + + test('should not perform any work when remote checksums are empty', async () => { + // Given + const files = new Map([]) + const defaultThemeFileSystem = fakeThemeFileSystem('tmp', files) + const emptyRemoteChecksums: Checksum[] = [] + + // When + await reconcileAndWaitForReconciliationFinish( + developmentTheme, + adminSession, + emptyRemoteChecksums, + defaultThemeFileSystem, + defaultOptions, + ) + + // Then + expect(renderSelectPrompt).not.toHaveBeenCalled() + }) }) async function reconcileAndWaitForReconciliationFinish(...args: Parameters) { - const {reconciliationFinishedPromise} = await reconcileJsonFiles(...args) - return reconciliationFinishedPromise + const {workPromise} = await reconcileJsonFiles(...args) + return workPromise }