Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Themes] Theme Dev - Editor Sync - Fix Hanging Theme Reconciliation Progress Bar and User Inputs #4392

Merged
merged 6 commits into from
Sep 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions packages/theme/src/cli/services/dev.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand All @@ -17,17 +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<string, ThemeAsset>([])
const defaultThemeFileSystem = fakeThemeFileSystem('tmp', files)
const initialRemoteChecksums = [{checksum: '1', key: 'templates/asset.json'}]

vi.mocked(reconcileJsonFiles).mockResolvedValue(undefined)
vi.mocked(fetchChecksums).mockResolvedValue([{checksum: '2', key: 'templates/asset.json'}])

// When
await reconcileAndPollThemeEditorChanges(
const {workPromise, updatedRemoteChecksumsPromise} = await reconcileAndPollThemeEditorChanges(
developmentTheme,
adminSession,
initialRemoteChecksums,
Expand All @@ -38,6 +43,8 @@ describe('reconcileAndPollThemeEditorChanges', async () => {
only: [],
},
)
await workPromise
await updatedRemoteChecksumsPromise

// Then
expect(pollThemeEditorChanges).toHaveBeenCalledWith(
Expand All @@ -53,34 +60,6 @@ describe('reconcileAndPollThemeEditorChanges', async () => {
)
})

test('should not call reconcileJsonFiles when remote theme contains no files', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test and logic moved to theme-reconciliation

// Given
const files = new Map<string, ThemeAsset>([])
const defaultThemeFileSystem = fakeThemeFileSystem('tmp', files)
const emptyRemoteChecksums: [] = []
const newFileSystem = fakeThemeFileSystem('tmp', new Map<string, ThemeAsset>([]))

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<string, ThemeAsset>([])
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
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'
import {outputDebug} from '@shopify/cli-kit/node/output'

export const LOCAL_STRATEGY = 'local'
export const REMOTE_STRATEGY = 'remote'
Expand All @@ -22,16 +22,20 @@ export async function reconcileAndPollThemeEditorChanges(
ignore: string[]
only: string[]
},
) {
): Promise<{
updatedRemoteChecksumsPromise: Promise<Checksum[]>
workPromise: Promise<void>
}> {
outputDebug('Initiating theme asset reconciliation process')
await localThemeFileSystem.ready()

if (remoteChecksums.length !== 0) {
await reconcileJsonFiles(targetTheme, session, remoteChecksums, localThemeFileSystem, options)
}
const {workPromise} = await reconcileJsonFiles(targetTheme, session, remoteChecksums, localThemeFileSystem, 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
return {updatedRemoteChecksumsPromise, workPromise}
}
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,10 @@ describe('setupDevServer', () => {
...filters,
},
}
vi.mocked(reconcileAndPollThemeEditorChanges).mockResolvedValue({
updatedRemoteChecksumsPromise: Promise.resolve([]),
workPromise: Promise.resolve(),
})

// When
await setupDevServer(developmentTheme, context).workPromise
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
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'
import {fetchChecksums} from '@shopify/cli-kit/node/themes/api'
import {createServer} from 'node:http'
Expand All @@ -28,32 +29,61 @@ 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,
ignore: ctx.options.ignore,
only: ctx.options.only,
})
: remoteChecksums,
handleThemeEditorSync(theme, ctx, remoteChecksums),
)

const uploadPromise = reconcilePromise.then(async (remoteChecksums: Checksum[]) =>
uploadTheme(theme, ctx.session, remoteChecksums, ctx.localThemeFileSystem, {
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: uploadPromise.then((result) => result.workPromise),
renderProgress: async () => {
if (ctx.options.themeEditorSync) {
const {workPromise} = await reconcilePromise
await renderTasksToStdErr([
{
title: 'Performing file synchronization. This may take a while...',
task: async () => {
await workPromise
},
},
])
}

const {renderThemeSyncProgress} = await uploadPromise

await renderThemeSyncProgress()
},
}
}

function handleThemeEditorSync(
theme: Theme,
ctx: DevServerContext,
remoteChecksums: Checksum[],
): Promise<{
updatedRemoteChecksumsPromise: Promise<Checksum[]>
workPromise: Promise<void>
}> {
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({
updatedRemoteChecksumsPromise: Promise.resolve(remoteChecksums),
workPromise: Promise.resolve(),
})
}
}

interface DevelopmentServerInstance {
close: () => Promise<void>
}
Expand Down
Loading
Loading