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

[Theme] Speed up theme synchronization #4362

Merged
merged 20 commits into from
Aug 27, 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
10 changes: 7 additions & 3 deletions packages/theme/src/cli/services/dev.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,13 @@ describe('dev', () => {
vi.mocked(isStorefrontPasswordProtected).mockResolvedValue(true)
vi.mocked(ensureValidPassword).mockResolvedValue('valid-password')
vi.mocked(fetchChecksums).mockResolvedValue([])
vi.mocked(mountThemeFileSystem).mockResolvedValue(localThemeFileSystem)
vi.mocked(setupDevServer).mockResolvedValue({dispatch: () => {}, start: async () => ({close: async () => {}})})
vi.mocked(mountThemeFileSystem).mockReturnValue(localThemeFileSystem)
vi.mocked(setupDevServer).mockReturnValue({
workPromise: Promise.resolve(),
renderDevSetupProgress: () => Promise.resolve(),
dispatchEvent: () => {},
serverStart: async () => ({close: async () => {}}),
})

const devOptions = {...options, storePassword: 'wrong-password', 'dev-preview': true, 'theme-editor-sync': true}

Expand All @@ -61,7 +66,6 @@ describe('dev', () => {
storefrontToken: 'my-storefront-token',
expiresAt: expect.any(Date),
},
remoteChecksums: [],
localThemeFileSystem,
directory: 'my-directory',
options: {
Expand Down
22 changes: 12 additions & 10 deletions packages/theme/src/cli/services/dev.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import {execCLI2} from '@shopify/cli-kit/node/ruby'
import {outputDebug, outputInfo} from '@shopify/cli-kit/node/output'
import {useEmbeddedThemeCLI} from '@shopify/cli-kit/node/context/local'
import {Theme} from '@shopify/cli-kit/node/themes/types'
import {fetchChecksums} from '@shopify/cli-kit/node/themes/api'
import {checkPortAvailability, getAvailableTCPPort} from '@shopify/cli-kit/node/tcp'
import {AbortError} from '@shopify/cli-kit/node/error'
import {openURL} from '@shopify/cli-kit/node/system'
Expand Down Expand Up @@ -52,9 +51,10 @@ export async function dev(options: DevOptions) {
return
}

const storefrontPassword = (await isStorefrontPasswordProtected(options.adminSession.storeFqdn))
? await ensureValidPassword(options.storePassword, options.adminSession.storeFqdn)
: undefined
const storefrontPasswordPromise = isStorefrontPasswordProtected(options.adminSession.storeFqdn).then(
(needsPassword) =>
needsPassword ? ensureValidPassword(options.storePassword, options.adminSession.storeFqdn) : undefined,
)

if (options.flagsToPass.includes('--poll')) {
renderWarning({
Expand All @@ -64,12 +64,10 @@ export async function dev(options: DevOptions) {

outputInfo('This feature is currently in development and is not ready for use or testing yet.')

const remoteChecksums = await fetchChecksums(options.theme.id, options.adminSession)
const localThemeFileSystem = await mountThemeFileSystem(options.directory)
const localThemeFileSystem = mountThemeFileSystem(options.directory)
const session: DevServerSession = {
...options.adminSession,
storefrontToken: options.storefrontToken,
storefrontPassword,
expiresAt: new Date(),
}

Expand All @@ -84,7 +82,6 @@ export async function dev(options: DevOptions) {

const ctx: DevServerContext = {
session,
remoteChecksums,
localThemeFileSystem,
directory: options.directory,
options: {
Expand All @@ -99,8 +96,13 @@ export async function dev(options: DevOptions) {
},
}

const server = await setupDevServer(options.theme, ctx)
await server.start()
const {serverStart, renderDevSetupProgress} = setupDevServer(options.theme, ctx)

const storefrontPassword = await storefrontPasswordPromise
session.storefrontPassword = storefrontPassword

await renderDevSetupProgress()
await serverStart()

renderLinks(options.store, String(options.theme.id), host, port)
if (options.open) {
Expand Down
6 changes: 5 additions & 1 deletion packages/theme/src/cli/services/push.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,11 @@ describe('push', () => {
const adminSession = {token: '', storeFqdn: ''}

beforeEach(() => {
vi.mocked(uploadTheme).mockResolvedValue(new Map())
vi.mocked(uploadTheme).mockResolvedValue({
workPromise: Promise.resolve(),
uploadResults: new Map(),
renderThemeSyncProgress: () => Promise.resolve(),
})
})

test('should call publishTheme if publish flag is provided', async () => {
Expand Down
14 changes: 11 additions & 3 deletions packages/theme/src/cli/services/push.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,23 @@ interface JsonOutput {

export async function push(theme: Theme, session: AdminSession, options: PushOptions) {
const themeChecksums = await fetchChecksums(theme.id, session)
const themeFileSystem = await mountThemeFileSystem(options.path)
const themeFileSystem = mountThemeFileSystem(options.path)

const results = await uploadTheme(theme, session, themeChecksums, themeFileSystem, options)
const {uploadResults, renderThemeSyncProgress} = await uploadTheme(
Copy link
Contributor

@jamesmengo jamesmengo Aug 22, 2024

Choose a reason for hiding this comment

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

This isn't related to the changes in this PR, but this method is hanging / not terminating when I 🎩!

These changes will resolve that issue for us - specifically this commit

Let's make sure that we're able to merge the changes / commit I linked above before the next to avoid breaking push

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see the changes to fix this in your links 🤔
In any case, this problem should be fixed here: e3f82ff

Basically, we only defer the delete job if specified in the function, so that the push command can await for everything. Can you check it? 🙏

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry - didn't see this. Push and pull are both working properly 👌🏻 !

theme,
session,
themeChecksums,
themeFileSystem,
options,
)

await renderThemeSyncProgress()

if (options.publish) {
await publishTheme(theme.id, session)
}

await handlePushOutput(results, theme, session, options)
await handlePushOutput(uploadResults, theme, session, options)
}

function hasUploadErrors(results: Map<string, Result>): boolean {
Expand Down
2 changes: 1 addition & 1 deletion packages/theme/src/cli/utilities/asset-checksum.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import {isThemeAsset, isJson, isTextFile} from './theme-fs.js'
import {Checksum} from '@shopify/cli-kit/node/themes/types'
import {fileHash} from '@shopify/cli-kit/node/crypto'

export async function calculateChecksum(fileKey: string, fileContent: string | Buffer | undefined) {
export function calculateChecksum(fileKey: string, fileContent: string | Buffer | undefined) {
let content = fileContent

if (!content) return ''
Expand Down
8 changes: 4 additions & 4 deletions packages/theme/src/cli/utilities/asset-ignore.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import {fileExists, readFile, matchGlob as originalMatchGlob} from '@shopify/cli-kit/node/fs'
import {outputDebug, outputWarn} from '@shopify/cli-kit/node/output'
import {joinPath} from '@shopify/cli-kit/node/path'
import {Checksum, ThemeFileSystem} from '@shopify/cli-kit/node/themes/types'
import {ThemeFileSystem} from '@shopify/cli-kit/node/themes/types'

const SHOPIFY_IGNORE = '.shopifyignore'

export async function applyIgnoreFilters(
themeChecksums: Checksum[],
export async function applyIgnoreFilters<T extends {key: string}>(
themeChecksums: T[],
themeFileSystem: ThemeFileSystem,
options: {ignore?: string[]; only?: string[]} = {},
) {
Expand All @@ -24,7 +24,7 @@ export async function applyIgnoreFilters(
}

function filterBy(patterns: string[], type: string, invertMatch = false) {
return ({key}: Checksum) => {
return ({key}: {key: string}) => {
if (patterns.length === 0) return true

const match = patterns.some((pattern) => matchGlob(key, pattern) || regexMatch(key, pattern))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,6 @@ function createTestContext(options?: {files?: [string, string][]}) {

const ctx: DevServerContext = {
session: {storefrontToken: '', token: '', storeFqdn: 'my-store.myshopify.com', expiresAt: new Date()},
remoteChecksums: [],
localThemeFileSystem,
directory: 'tmp',
options: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,15 @@ import {getClientScripts, HotReloadEvent} from './client.js'
import {render} from '../storefront-renderer.js'
import {patchRenderingResponse} from '../proxy.js'
import {prettifySyntaxErrors} from '../html.js'
import {createEventStream, defineEventHandler, getProxyRequestHeaders, getQuery, sendError, type H3Error} from 'h3'
import {
createError,
createEventStream,
defineEventHandler,
getProxyRequestHeaders,
getQuery,
sendError,
type H3Error,
} from 'h3'
import {renderWarning} from '@shopify/cli-kit/node/ui'
import {extname, joinPath} from '@shopify/cli-kit/node/path'
import {parseJSON} from '@shopify/theme-check-node'
Expand Down Expand Up @@ -188,6 +196,14 @@ export function getHotReloadHandler(theme: Theme, ctx: DevServerContext) {
replaceTemplates,
})
.then(async (response) => {
if (!response.ok) {
throw createError({
status: response.status,
statusText: response.statusText,
data: {requestId: response.headers.get('x-request-id'), url: response.url},
})
}

const html = await patchRenderingResponse(ctx, event, response)
return prettifySyntaxErrors(html)
})
Expand Down
2 changes: 2 additions & 0 deletions packages/theme/src/cli/utilities/theme-environment/proxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
setResponseHeaders,
setResponseHeader,
removeResponseHeader,
setResponseStatus,
} from 'h3'
import {lookupMimeType} from '@shopify/cli-kit/node/mimes'
import {extname} from '@shopify/cli-kit/node/path'
Expand Down Expand Up @@ -115,6 +116,7 @@ function patchCookieDomains(cookieHeader: string[], ctx: DevServerContext) {
* and fix domain inconsistencies between remote instance and local dev.
*/
export async function patchRenderingResponse(ctx: DevServerContext, event: H3Event, response: NodeResponse) {
setResponseStatus(event, response.status, response.statusText)
setResponseHeaders(event, Object.fromEntries(response.headers.entries()))
patchProxiedResponseHeaders(ctx, event, response)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ describe('reconcileAndPollThemeEditorChanges', async () => {

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

// When
await reconcileAndPollThemeEditorChanges(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ export async function reconcileAndPollThemeEditorChanges(

const updatedRemoteChecksums = await fetchChecksums(targetTheme.id, session)

const themeFileSystem = await mountThemeFileSystem(localThemeFileSystem.root)
const themeFileSystem = mountThemeFileSystem(localThemeFileSystem.root)
pollThemeEditorChanges(targetTheme, session, updatedRemoteChecksums, themeFileSystem, options)

return updatedRemoteChecksums
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,6 @@ export async function render(session: DevServerSession, context: DevServerRender
const requestId = response.headers.get('x-request-id')
outputDebug(`← ${response.status} (request_id: ${requestId})`)

if (!response.ok) {
throw createError({
status: response.status,
statusText: response.statusText,
data: {requestId, url},
})
}

return response
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,21 +1,40 @@
import {reconcileAndPollThemeEditorChanges} from './remote-theme-watcher.js'
import {DevServerContext} from './types.js'
import {setupDevServer} from './theme-environment.js'
import {render} from './storefront-renderer.js'
import {reconcileAndPollThemeEditorChanges} from './remote-theme-watcher.js'
import {uploadTheme} from '../theme-uploader.js'
import {fakeThemeFileSystem} from '../theme-fs/theme-fs-mock-factory.js'
import {DEVELOPMENT_THEME_ROLE} from '@shopify/cli-kit/node/themes/utils'
import {describe, expect, test, vi} from 'vitest'
import {describe, expect, test, vi, beforeEach} from 'vitest'
import {buildTheme} from '@shopify/cli-kit/node/themes/factories'
import {Response as NodeResponse} from '@shopify/cli-kit/node/http'
import {createEvent} from 'h3'
import {IncomingMessage, ServerResponse} from 'node:http'
import {Socket} from 'node:net'

vi.mock('@shopify/cli-kit/node/themes/api', () => ({fetchChecksums: () => Promise.resolve([])}))
vi.mock('./remote-theme-watcher.js')
vi.mock('../theme-uploader.js')
vi.mock('./storefront-renderer.js')

// Vitest is resetting this mock between tests due to a global config `mockReset: true`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I couldn't get this working either - Please update me if you figure out another way around this!

// For some reason we need to re-mock it here and in beforeEach:
vi.mock('../theme-uploader.js', async () => {
return {
uploadTheme: vi.fn(() => {
return {
workPromise: Promise.resolve(),
uploadResults: new Map(),
renderThemeSyncProgress: () => Promise.resolve(),
}
}),
}
})
beforeEach(() => {
vi.mocked(uploadTheme).mockImplementation(() => {
return {workPromise: Promise.resolve(), uploadResults: new Map(), renderThemeSyncProgress: () => Promise.resolve()}
})
})

describe('startDevServer', () => {
const developmentTheme = buildTheme({id: 1, name: 'Theme', role: DEVELOPMENT_THEME_ROLE})!
const localFiles = new Map([
Expand All @@ -41,7 +60,6 @@ describe('startDevServer', () => {
const localThemeFileSystem = fakeThemeFileSystem('tmp', localFiles)
const defaultServerContext: DevServerContext = {
session: {storefrontToken: '', token: '', storeFqdn: 'my-store.myshopify.com', expiresAt: new Date()},
remoteChecksums: [],
localThemeFileSystem,
directory: 'tmp',
options: {
Expand All @@ -63,20 +81,15 @@ describe('startDevServer', () => {
}

// When
await setupDevServer(developmentTheme, context)
await setupDevServer(developmentTheme, context).workPromise

// Then
expect(uploadTheme).toHaveBeenCalledWith(
developmentTheme,
context.session,
context.remoteChecksums,
context.localThemeFileSystem,
{
ignore: ['assets/*.json'],
nodelete: true,
only: ['templates/*.liquid'],
},
)
expect(uploadTheme).toHaveBeenCalledWith(developmentTheme, context.session, [], context.localThemeFileSystem, {
ignore: ['assets/*.json'],
nodelete: true,
only: ['templates/*.liquid'],
deferPartialWork: true,
})
})

test('should initialize theme editor sync if themeEditorSync flag is passed', async () => {
Expand All @@ -90,13 +103,13 @@ describe('startDevServer', () => {
}

// When
await setupDevServer(developmentTheme, context)
await setupDevServer(developmentTheme, context).workPromise

// Then
expect(reconcileAndPollThemeEditorChanges).toHaveBeenCalledWith(
developmentTheme,
context.session,
context.remoteChecksums,
[],
context.localThemeFileSystem,
{
ignore: ['assets/*.json'],
Expand All @@ -111,19 +124,21 @@ describe('startDevServer', () => {
const context = {...defaultServerContext, options: {...defaultServerContext.options, noDelete: true}}

// When
await setupDevServer(developmentTheme, context)
await setupDevServer(developmentTheme, context).workPromise

// Then
expect(uploadTheme).toHaveBeenCalledWith(developmentTheme, context.session, [], context.localThemeFileSystem, {
ignore: ['assets/*.json'],
nodelete: true,
only: ['templates/*.liquid'],
deferPartialWork: true,
})
})

describe('request handling', async () => {
const context = {...defaultServerContext}
const {dispatch} = await setupDevServer(developmentTheme, context)
const server = setupDevServer(developmentTheme, context)

const html = String.raw
const decoder = new TextDecoder()

Expand Down Expand Up @@ -154,7 +169,7 @@ describe('startDevServer', () => {
return resEnd(content)
}

await dispatch(event)
await server.dispatchEvent(event)
return {res, status: res.statusCode, body}
}

Expand Down
Loading