From f7794158d34f095fe723dfc0e9a6a68377b3ef23 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Wed, 21 Oct 2026 18:06:30 +0200 Subject: [PATCH 1/4] Add Dev Instance support to theme dev --- .changeset/theme-dev-instance.md | 5 + docs/cli/dev-instance-agent-workflow.md | 16 ++ docs/cli/theme-dev-agent-workflow.md | 36 ++++ .../src/cli/services/dev.integration.test.ts | 59 ++++++ packages/theme/src/cli/services/dev.test.ts | 183 ++++++++++++++++-- packages/theme/src/cli/services/dev.ts | 103 +++++++--- .../theme/src/cli/services/dev/output.test.ts | 102 ++++++++++ packages/theme/src/cli/services/dev/output.ts | 44 +++++ .../dev-server-session.test.ts | 19 ++ .../theme-environment/dev-server-session.ts | 6 +- .../theme-environment/remote-theme-watcher.ts | 5 +- .../theme-environment.test.ts | 21 ++ .../theme-environment/theme-environment.ts | 39 ++-- .../theme-environment/theme-polling.test.ts | 19 +- .../theme-environment/theme-polling.ts | 14 +- .../cli/utilities/theme-environment/types.ts | 3 + .../theme/src/cli/utilities/theme-fs.test.ts | 26 +++ packages/theme/src/cli/utilities/theme-fs.ts | 14 +- .../fixtures/dev-instance-dependencies.js | 43 ++++ .../test/fixtures/dev-instance-loader.js | 22 +++ 20 files changed, 713 insertions(+), 66 deletions(-) create mode 100644 .changeset/theme-dev-instance.md create mode 100644 docs/cli/theme-dev-agent-workflow.md create mode 100644 packages/theme/src/cli/services/dev.integration.test.ts create mode 100644 packages/theme/src/cli/services/dev/output.test.ts create mode 100644 packages/theme/src/cli/services/dev/output.ts create mode 100644 packages/theme/test/fixtures/dev-instance-dependencies.js create mode 100644 packages/theme/test/fixtures/dev-instance-loader.js diff --git a/.changeset/theme-dev-instance.md b/.changeset/theme-dev-instance.md new file mode 100644 index 00000000000..5262b8748ca --- /dev/null +++ b/.changeset/theme-dev-instance.md @@ -0,0 +1,5 @@ +--- +'@shopify/theme': minor +--- + +Record `theme dev` sessions, preview URLs, and JSONL logs so agents can reuse a running session. diff --git a/docs/cli/dev-instance-agent-workflow.md b/docs/cli/dev-instance-agent-workflow.md index b088f608d45..50c251820bc 100644 --- a/docs/cli/dev-instance-agent-workflow.md +++ b/docs/cli/dev-instance-agent-workflow.md @@ -68,3 +68,19 @@ With `--json`, dev stream records use identical JSONL bytes in stdout and the se log. The log contains process output, preview URLs, and session status messages. CLI setup messages and other diagnostics use stderr. Fatal CLI errors use the standard JSON error output on stdout and are not added to the session log. + +## Theme dev + +`shopify theme dev` owns one session per store and theme ID in a directory. +Sessions for different listings of the same store and theme cannot run together. +Different stores or theme IDs can run together. + +The instance details include `store`, `themeId`, `themeName`, `listing` when set, +`host`, `port`, and `urls` with `local`, `preview`, `themeEditor`, and `giftCard` +links. URLs are added during setup. The instance status becomes `running` after +the server is listening and initial synchronization has completed. + +The log includes theme dev messages, file synchronization results, request lines, +errors, and setup progress. Theme records omit the redundant `source` field. +Authentication before the session and fatal CLI errors reported after it are +outside this log. diff --git a/docs/cli/theme-dev-agent-workflow.md b/docs/cli/theme-dev-agent-workflow.md new file mode 100644 index 00000000000..412727909d0 --- /dev/null +++ b/docs/cli/theme-dev-agent-workflow.md @@ -0,0 +1,36 @@ +# Theme dev session files + +`shopify theme dev` writes instance metadata and a JSONL log under +`/.shopify/dev/`. Agents can read these files to find a session started in +another terminal. No IPC endpoint or extra command is required. + +Each instance uses the store and theme ID as its key. Two sessions for the same +store and theme in the same directory cannot run together, including sessions +with different listings. Different stores or theme IDs can run together. + +The instance JSON includes the PID, `sessionId`, `logFile`, and `status`: + +- `starting`: the CLI is preparing the session or synchronizing the theme. +- `running`: the server is listening and initial synchronization has completed. + +`details` contains `store`, `themeId`, `themeName`, `listing` when set, `host`, +`port`, and `urls` with `local`, `preview`, `themeEditor`, and `giftCard` links. +The URLs are added during setup. `running` is not a health check or proof that +later edits have reached the store. Read new output and test the result. + +The log contains theme dev messages, file synchronization results, request lines, +errors, and setup progress. Each complete line contains `timestamp`, `source`, +and `message`. Colors and progress redraws are excluded. Authentication before +the dev session and fatal CLI errors reported after it are outside this log. + +Read the instance JSON's `logFile` field to find the log. Keep a byte offset to +read new complete lines. Reset the offset if `sessionId` changes: each new run +clears the log. Logs have no size limit or rotation. + +Before starting another session, check the PID and lock heartbeat. If a second +start reports `dev_instance_already_running`, use the reported session files. +Do not stop a session started by the developer. The shared lock recovery and +interrupted-start cleanup rules are described in [App dev session files](app-dev-agent-workflow.md). + +Ctrl+C and SIGTERM stop dev work and drain queued log messages before exit. +Normal shutdown removes the instance JSON and lock, and retains the log. diff --git a/packages/theme/src/cli/services/dev.integration.test.ts b/packages/theme/src/cli/services/dev.integration.test.ts new file mode 100644 index 00000000000..28e9fe13ce9 --- /dev/null +++ b/packages/theme/src/cli/services/dev.integration.test.ts @@ -0,0 +1,59 @@ +import {test, expect} from 'vitest' +import {captureOutputWithExitCode} from '@shopify/cli-kit/node/system' +import {fileExists, inTemporaryDirectory, readFile} from '@shopify/cli-kit/node/fs' +import {devInstancePaths} from '@shopify/cli-kit/node/dev-instance' +import {fileURLToPath} from 'node:url' + +const bootstrapUrl = new URL('../../../../cli/src/bootstrap.ts', import.meta.url).href +const serviceUrl = new URL('./dev.ts', import.meta.url).href +const loaderUrl = new URL('../../../test/fixtures/dev-instance-loader.js', import.meta.url).href +const tsconfigPath = fileURLToPath(new URL('../../../../app/test/fixtures/tsconfig.json', import.meta.url)) + +test.skipIf(process.platform === 'win32').each(['SIGINT', 'SIGTERM'])( + '%s with bootstrap flushes theme output and exits on the first signal', + {timeout: 60000}, + async (signal) => { + await inTemporaryDirectory(async (directory) => { + const script = ` + await import(${JSON.stringify(bootstrapUrl)}) + const {dev} = await import(${JSON.stringify(serviceUrl)}) + await dev({ + directory: ${JSON.stringify(directory)}, store: 'fixture.myshopify.com', + adminSession: {storeFqdn: 'fixture.myshopify.com', token: 'fixture'}, commandConfig: {}, + theme: {id: 123, name: 'Fixture', role: 'development'}, + open: false, force: false, noDelete: false, ignore: [], only: [], + 'standard-events-inspector': false, 'theme-editor-sync': false, + 'live-reload': 'hot-reload', 'error-overlay': 'default', + }) + ` + const result = await captureOutputWithExitCode( + process.execPath, + ['--loader', 'ts-node/esm', '--loader', loaderUrl, '--input-type=module', '--eval', script], + { + env: { + ...process.env, + NODE_NO_WARNINGS: '1', + SHOPIFY_UNIT_TEST: '0', + SHOPIFY_CLI_ENV: 'development', + SHOPIFY_CLI_NO_ANALYTICS: '1', + CI: '1', + TS_NODE_PROJECT: tsconfigPath, + THEME_DEV_TEST_SIGNAL: signal, + }, + }, + ) + expect(result.exitCode, result.stderr).toBe(0) + const paths = devInstancePaths({command: 'theme dev', directory, key: 'fixture.myshopify.com:123'}) + const messages = (await readFile(paths.logFile)) + .trim() + .split('\n') + .map((line) => JSON.parse(line).message) + expect(messages.filter((message: string) => message.startsWith('line '))).toEqual( + Array.from({length: 1000}, (_, index) => `line ${index}`), + ) + expect(messages.at(-1)).toBe('Stopped') + await expect(fileExists(paths.infoFile)).resolves.toBe(false) + await expect(fileExists(paths.lockFile)).resolves.toBe(false) + }) + }, +) diff --git a/packages/theme/src/cli/services/dev.test.ts b/packages/theme/src/cli/services/dev.test.ts index c4810488bdf..8a9649e5e80 100644 --- a/packages/theme/src/cli/services/dev.test.ts +++ b/packages/theme/src/cli/services/dev.test.ts @@ -13,7 +13,14 @@ import {reportAnalyticsEvent} from '@shopify/cli-kit/node/analytics' import {addPublicMetadata, addSensitiveMetadata} from '@shopify/cli-kit/node/metadata' import {getAvailableTCPPort, checkPortAvailability} from '@shopify/cli-kit/node/tcp' import {Config} from '@oclif/core' +import {readDevInstance, devInstancePaths, acquireDevInstance} from '@shopify/cli-kit/node/dev-instance' +import {fileExists, readFile} from '@shopify/cli-kit/node/fs' +import {withCapturedStandardStreams} from '@shopify/cli-kit/node/testing/output' +import {outputInfo} from '@shopify/cli-kit/node/output' +import {joinPath} from '@shopify/cli-kit/node/path' import readline from 'readline' +import {mkdtemp, rm} from 'node:fs/promises' +import {tmpdir} from 'node:os' vi.mock('@shopify/cli-kit/node/ui') vi.mock('@shopify/cli-kit/node/colors', () => ({ @@ -64,6 +71,7 @@ vi.mock('../utilities/theme-environment/crawler-signature.js', async (realImport } }) +const processEvents: NodeJS.EventEmitter = process const store = 'my-store.myshopify.com' const theme = buildTheme({id: 123, name: 'My Theme', role: DEVELOPMENT_THEME_ROLE})! const crawlerSignatureHeaders = { @@ -314,7 +322,8 @@ describe('dev() Ctrl-C analytics', () => { only: [], } - beforeEach(() => { + beforeEach(async () => { + baseOptions.directory = await mkdtemp(joinPath(tmpdir(), 'theme-dev-test-')) vi.mocked(hasRequiredThemeDirectories).mockResolvedValue(true) vi.mocked(isStorefrontPasswordProtected).mockResolvedValue(false) vi.mocked(fetchOrCreateCrawlerSignatureHeaders).mockResolvedValue(crawlerSignatureHeaders) @@ -330,24 +339,26 @@ describe('dev() Ctrl-C analytics', () => { }) vi.mocked(setupDevServer).mockReturnValue({ serverStart: vi.fn().mockResolvedValue(undefined), + serverStop: vi.fn().mockResolvedValue(undefined), + workPromise: Promise.resolve(), renderDevSetupProgress: vi.fn().mockResolvedValue(undefined), backgroundJobPromise, resolveBackgroundJob: resolveBackgroundJob!, - dispatchEvent: vi.fn(), + dispatchEvent: vi.fn(async () => undefined), } as any) exitSpy = vi.spyOn(process, 'exit').mockImplementation(() => undefined as never) }) - afterEach(() => { + afterEach(async () => { exitSpy.mockRestore() + await rm(baseOptions.directory, {recursive: true, force: true}) }) test('Ctrl-C path: reports analytics once, even if reportDevAnalytics is invoked again', async () => { const devPromise = dev(baseOptions) - // Flush microtasks so the Promise.all is awaiting backgroundJobPromise. - await new Promise((resolve) => setImmediate(resolve)) + await vi.waitFor(() => expect(setupDevServer).toHaveBeenCalled()) // Simulate Ctrl-C by resolving the background job. resolveBackgroundJob() @@ -378,7 +389,7 @@ describe('dev() Ctrl-C analytics', () => { test('enables the standard events dev bundle by default', async () => { const devPromise = dev({...baseOptions, 'standard-events-inspector': false}) - await new Promise((resolve) => setImmediate(resolve)) + await vi.waitFor(() => expect(setupDevServer).toHaveBeenCalled()) resolveBackgroundJob() await devPromise @@ -397,7 +408,7 @@ describe('dev() Ctrl-C analytics', () => { test('propagates the inspector option to the dev server context', async () => { const devPromise = dev({...baseOptions, 'standard-events-inspector': true}) - await new Promise((resolve) => setImmediate(resolve)) + await vi.waitFor(() => expect(setupDevServer).toHaveBeenCalled()) resolveBackgroundJob() await devPromise @@ -413,7 +424,9 @@ describe('dev() Ctrl-C analytics', () => { ) }) - test('does not configure stdin when input is unavailable', async () => { + test.each([false, true])('restores stdin after stopping with input available: %s', async (acceptsInput) => { + vi.mocked(terminalSupportsPrompting).mockReturnValue(acceptsInput) + const listenerCount = process.stdin.listenerCount('keypress') const emitKeypressEventsSpy = vi.spyOn(readline, 'emitKeypressEvents') const stdinOnSpy = vi.spyOn(process.stdin, 'on') const originalSetRawModeDescriptor = Object.getOwnPropertyDescriptor(process.stdin, 'setRawMode') @@ -423,14 +436,25 @@ describe('dev() Ctrl-C analytics', () => { try { const devPromise = dev(baseOptions) - await new Promise((resolve) => setImmediate(resolve)) + await vi.waitFor(() => expect(setupDevServer).toHaveBeenCalled()) - resolveBackgroundJob() + if (acceptsInput) { + await vi.waitFor(() => expect(setRawMode).toHaveBeenCalledWith(true)) + process.stdin.emit('keypress', '\u0003', {ctrl: true, name: 'c'}) + } else { + resolveBackgroundJob() + } await devPromise - expect(emitKeypressEventsSpy).not.toHaveBeenCalled() - expect(stdinOnSpy).not.toHaveBeenCalledWith('keypress', expect.any(Function)) - expect(setRawMode).not.toHaveBeenCalled() + if (acceptsInput) { + expect(emitKeypressEventsSpy).toHaveBeenCalled() + expect(setRawMode.mock.calls).toEqual([[true], [false]]) + } else { + expect(emitKeypressEventsSpy).not.toHaveBeenCalled() + expect(stdinOnSpy).not.toHaveBeenCalledWith('keypress', expect.any(Function)) + expect(setRawMode).not.toHaveBeenCalled() + } + expect(process.stdin.listenerCount('keypress')).toBe(listenerCount) } finally { if (originalSetRawModeDescriptor) { Object.defineProperty(process.stdin, 'setRawMode', originalSetRawModeDescriptor) @@ -439,6 +463,127 @@ describe('dev() Ctrl-C analytics', () => { } } }) + + test.each(['SIGINT', 'SIGTERM', 'shopify:signal'])( + '%s drains output, closes the server, and releases ownership', + async (event) => { + await withCapturedStandardStreams(async () => { + const identity = { + command: 'theme dev', + directory: baseOptions.directory, + key: `${baseOptions.store}:${theme.id}`, + } + const paths = devInstancePaths(identity) + const listenerCounts = ['SIGINT', 'SIGTERM', 'shopify:signal'].map((name) => process.listenerCount(name)) + const {promise: sendOutput, resolve: continueOutput} = Promise.withResolvers() + const serverStop = vi.fn(async () => { + await expect(fileExists(paths.infoFile)).resolves.toBe(true) + outputInfo('Stopped') + }) + vi.mocked(setupDevServer).mockImplementationOnce(() => { + const backgroundJobPromise = (async () => { + await sendOutput + for (let index = 0; index < 1000; index++) outputInfo(`Synced file ${index}`) + const stop = processEvents.listeners(event).at(-1)! + stop() + })() + return { + serverStart: vi.fn().mockResolvedValue(undefined), + serverStop, + workPromise: Promise.resolve(), + renderDevSetupProgress: vi.fn().mockResolvedValue(undefined), + backgroundJobPromise, + resolveBackgroundJob: vi.fn(), + dispatchEvent: vi.fn(async () => undefined), + } + }) + const running = dev(baseOptions) + try { + await vi.waitFor(async () => { + const info = await readDevInstance(identity) + expect(info?.status).toBe('running') + expect(info?.details).toMatchObject({ + store: baseOptions.store, + themeId: theme.id, + themeName: theme.name, + host: '127.0.0.1', + port: 9292, + urls: { + local: 'http://127.0.0.1:9292', + preview: `https://${baseOptions.store}/?preview_theme_id=${theme.id}`, + }, + }) + }) + await expect(dev(baseOptions)).rejects.toMatchObject({code: 'dev_instance_already_running'}) + expect(setupDevServer).toHaveBeenCalledTimes(1) + } finally { + continueOutput() + await running + } + expect(serverStop).toHaveBeenCalledOnce() + expect(exitSpy).toHaveBeenCalledWith(0) + await expect(fileExists(paths.infoFile)).resolves.toBe(false) + await expect(fileExists(paths.lockFile)).resolves.toBe(false) + const records = (await readFile(paths.logFile)) + .trim() + .split('\n') + .map((line) => JSON.parse(line)) + expect(records.map(({message}) => message)).toEqual([ + ...Array.from({length: 1000}, (_, index) => `Synced file ${index}`), + 'Stopped', + ]) + expect(['SIGINT', 'SIGTERM', 'shopify:signal'].map((name) => process.listenerCount(name))).toEqual( + listenerCounts, + ) + }) + }, + ) + + test('releases ownership after setup failure without recording a fatal CLI error', async () => { + const serverStop = vi.fn().mockResolvedValue(undefined) + vi.mocked(setupDevServer).mockReturnValueOnce({ + serverStart: vi.fn().mockRejectedValue(new Error('Cannot start server')), + serverStop, + workPromise: Promise.resolve(), + renderDevSetupProgress: vi.fn().mockResolvedValue(undefined), + backgroundJobPromise: new Promise(() => {}), + resolveBackgroundJob: vi.fn(), + dispatchEvent: vi.fn(async () => undefined), + }) + await expect(dev(baseOptions)).rejects.toThrow('Cannot start server') + const identity = {command: 'theme dev', directory: baseOptions.directory, key: `${baseOptions.store}:${theme.id}`} + expect(serverStop).toHaveBeenCalledOnce() + await expect(readDevInstance(identity)).resolves.toBeUndefined() + await expect(readFile(devInstancePaths(identity).logFile)).resolves.toBe('') + const nextInstance = await acquireDevInstance(identity) + await nextInstance.release() + }) + + test('stops during setup and does not start a server when the pending work later completes', async () => { + const {promise: progress, resolve: finishProgress} = Promise.withResolvers() + const serverStart = vi.fn().mockResolvedValue(undefined) + const serverStop = vi.fn().mockResolvedValue(undefined) + vi.mocked(setupDevServer).mockReturnValueOnce({ + serverStart, + serverStop, + workPromise: Promise.resolve(), + renderDevSetupProgress: () => progress, + backgroundJobPromise: new Promise(() => {}), + resolveBackgroundJob: vi.fn(), + dispatchEvent: vi.fn(async () => undefined), + }) + const running = dev(baseOptions) + await vi.waitFor(() => expect(setupDevServer).toHaveBeenCalled()) + const identity = {command: 'theme dev', directory: baseOptions.directory, key: `${baseOptions.store}:${theme.id}`} + expect((await readDevInstance(identity))?.status).toBe('starting') + processEvents.emit('shopify:signal') + await running + finishProgress() + await new Promise((resolve) => setImmediate(resolve)) + expect(serverStop).toHaveBeenCalledOnce() + expect(serverStart).not.toHaveBeenCalled() + await expect(readDevInstance(identity)).resolves.toBeUndefined() + }) }) describe('dev() port validation', () => { @@ -465,7 +610,8 @@ describe('dev() port validation', () => { only: [], } - beforeEach(() => { + beforeEach(async () => { + baseOptions.directory = await mkdtemp(joinPath(tmpdir(), 'theme-dev-test-')) vi.mocked(hasRequiredThemeDirectories).mockResolvedValue(true) vi.mocked(isStorefrontPasswordProtected).mockResolvedValue(false) vi.mocked(fetchOrCreateCrawlerSignatureHeaders).mockResolvedValue(crawlerSignatureHeaders) @@ -481,17 +627,20 @@ describe('dev() port validation', () => { }) vi.mocked(setupDevServer).mockReturnValue({ serverStart: vi.fn().mockResolvedValue(undefined), + serverStop: vi.fn().mockResolvedValue(undefined), + workPromise: Promise.resolve(), renderDevSetupProgress: vi.fn().mockResolvedValue(undefined), backgroundJobPromise, resolveBackgroundJob: resolveBackgroundJob!, - dispatchEvent: vi.fn(), + dispatchEvent: vi.fn(async () => undefined), } as any) exitSpy = vi.spyOn(process, 'exit').mockImplementation(() => undefined as never) }) - afterEach(() => { + afterEach(async () => { exitSpy.mockRestore() + await rm(baseOptions.directory, {recursive: true, force: true}) }) test('accepts a valid port and calls checkPortAvailability with it', async () => { @@ -499,7 +648,7 @@ describe('dev() port validation', () => { const devPromise = dev({...baseOptions, port: 9293}) - await new Promise((resolve) => setImmediate(resolve)) + await vi.waitFor(() => expect(setupDevServer).toHaveBeenCalled()) resolveBackgroundJob() await devPromise diff --git a/packages/theme/src/cli/services/dev.ts b/packages/theme/src/cli/services/dev.ts index 309211a7316..e97f96af26f 100644 --- a/packages/theme/src/cli/services/dev.ts +++ b/packages/theme/src/cli/services/dev.ts @@ -1,3 +1,4 @@ +import {withThemeDevOutput} from './dev/output.js' import {hasRequiredThemeDirectories, mountThemeFileSystem} from '../utilities/theme-fs.js' import {ensureDirectoryConfirmed} from '../utilities/theme-ui.js' import {setupDevServer} from '../utilities/theme-environment/theme-environment.js' @@ -24,6 +25,7 @@ import {reportAnalyticsEvent} from '@shopify/cli-kit/node/analytics' import {addPublicMetadata, addSensitiveMetadata} from '@shopify/cli-kit/node/metadata' import {hashString} from '@shopify/cli-kit/node/crypto' import chalk from '@shopify/cli-kit/node/colors' +import {withDevInstance, type DevInstance} from '@shopify/cli-kit/node/dev-instance' import {Config} from '@oclif/core' import readline from 'readline' @@ -88,6 +90,45 @@ export async function dev(options: DevOptions) { await ensureListingExists(options.directory, options.listing) } + await withDevInstance( + { + command: 'theme dev', + directory: options.directory, + key: `${options.store}:${options.theme.id}`, + details: { + store: options.store, + themeId: options.theme.id, + themeName: options.theme.name, + listing: options.listing, + }, + }, + async (instance) => { + const controller = new AbortController() + const stop = () => controller.abort() + const onInstanceError = () => controller.abort(instance.abortSignal.reason) + process.on('shopify:signal', stop) + process.on('SIGINT', stop) + process.on('SIGTERM', stop) + instance.abortSignal.addEventListener('abort', onInstanceError, {once: true}) + try { + await withThemeDevOutput(instance, () => runDev(options, instance, controller)) + instance.abortSignal.throwIfAborted() + } finally { + controller.abort() + process.off('shopify:signal', stop) + process.off('SIGINT', stop) + process.off('SIGTERM', stop) + instance.abortSignal.removeEventListener('abort', onInstanceError) + } + }, + ) + + await reportDevAnalytics(options.commandConfig, options.adminSession) + process.exit(0) +} + +async function runDev(options: DevOptions, instance: DevInstance, controller: AbortController) { + const {signal} = controller const [crawlerSignatureHeaders, isPasswordProtected] = await Promise.all([ fetchOrCreateCrawlerSignatureHeaders(options.adminSession), isStorefrontPasswordProtected(options.adminSession), @@ -96,12 +137,15 @@ export async function dev(options: DevOptions) { ? await ensureValidPassword(options.storePassword, options.adminSession.storeFqdn, crawlerSignatureHeaders) : undefined + if (signal.aborted) return + const localThemeExtensionFileSystem = emptyThemeExtFileSystem() const localThemeFileSystem = mountThemeFileSystem(options.directory, { filters: options, listing: options.listing, noDelete: options.noDelete, notify: options.notify, + signal, }) const host = options.host ?? DEFAULT_HOST @@ -124,14 +168,20 @@ export async function dev(options: DevOptions) { preview: `https://${options.store}/?preview_theme_id=${options.theme.id}`, } + await instance.update({details: {host, port, urls}}) + const session = await initializeDevServerSession( options.theme.id.toString(), options.adminSession, options.password, storefrontPassword, crawlerSignatureHeaders, + signal, ) + if (signal.aborted) return + const ctx: DevServerContext = { + signal, session, localThemeFileSystem, localThemeExtensionFileSystem, @@ -154,36 +204,43 @@ export async function dev(options: DevOptions) { }, } - const {serverStart, renderDevSetupProgress, backgroundJobPromise, resolveBackgroundJob} = setupDevServer( + const {serverStart, serverStop, workPromise, renderDevSetupProgress, backgroundJobPromise} = setupDevServer( options.theme, ctx, ) - const acceptsInput = terminalSupportsPrompting() - if (acceptsInput) { - readline.emitKeypressEvents(process.stdin) - const keypressHandler = createKeypressHandler(urls, ctx, resolveBackgroundJob) - process.stdin.on('keypress', keypressHandler) - } + const keypressHandler = createKeypressHandler(urls, ctx, () => controller.abort()) + const {promise: stopped, resolve: resolveStopped} = Promise.withResolvers() + const onAbort = () => resolveStopped() + signal.addEventListener('abort', onAbort, {once: true}) - await Promise.all([ - backgroundJobPromise, - renderDevSetupProgress() - .then(serverStart) - .then(() => { - if (acceptsInput) { - process.stdin.setRawMode(true) - } - renderLinks(urls) - if (options.open) { - openURLSafely(urls.local, 'development server') - } - }), - ]) + try { + if (acceptsInput) { + readline.emitKeypressEvents(process.stdin) + process.stdin.on('keypress', keypressHandler) + } - await reportDevAnalytics(options.commandConfig, options.adminSession) + const start = async () => { + await renderDevSetupProgress() + if (signal.aborted) return + await serverStart() + if (signal.aborted) return + if (acceptsInput) process.stdin.setRawMode(true) + renderLinks(urls) + if (options.open) openURLSafely(urls.local, 'development server') + await workPromise + if (signal.aborted) return + await instance.update({status: 'running'}) + } - process.exit(0) + await Promise.race([Promise.all([backgroundJobPromise, start()]), stopped]) + } finally { + controller.abort() + signal.removeEventListener('abort', onAbort) + process.stdin.off('keypress', keypressHandler) + if (acceptsInput) process.stdin.setRawMode(false) + await serverStop() + } } export async function reportDevAnalytics(config: Config, session: AdminSession): Promise { diff --git a/packages/theme/src/cli/services/dev/output.test.ts b/packages/theme/src/cli/services/dev/output.test.ts new file mode 100644 index 00000000000..8a5462c1da6 --- /dev/null +++ b/packages/theme/src/cli/services/dev/output.test.ts @@ -0,0 +1,102 @@ +import {withThemeDevOutput} from './output.js' +import {logRequestLine} from '../../utilities/log-request-line.js' +import {renderThrownError} from '../../utilities/errors.js' +import {inTemporaryDirectory, readFile} from '@shopify/cli-kit/node/fs' +import {withDevInstance} from '@shopify/cli-kit/node/dev-instance' +import {outputInfo} from '@shopify/cli-kit/node/output' +import {renderTasks, renderWarning} from '@shopify/cli-kit/node/ui' +import {withCapturedStandardStreams} from '@shopify/cli-kit/node/testing/output' +import {expect, test} from 'vitest' +import {createEvent} from 'h3' +import {IncomingMessage, ServerResponse, createServer} from 'node:http' +import {Socket} from 'node:net' +import {type AddressInfo} from 'node:net' + +test('records theme messages, task progress, sync errors, and late HTTP request output', async () => { + await inTemporaryDirectory(async (directory) => { + await withCapturedStandardStreams(async ({stderr}) => { + let logFile = '' + await withDevInstance({command: 'theme dev', directory}, async (instance) => { + logFile = instance.paths.logFile + await withThemeDevOutput(instance, async () => { + outputInfo('Synced » update assets/base.css') + renderWarning({body: 'A dev warning'}) + renderThrownError('Failed to upload assets/base.css.', new Error('Invalid CSS')) + await renderTasks([{title: 'Uploading theme files', task: async () => {}}], { + renderOptions: {patchConsole: false}, + }) + + // HTTP callbacks must retain the session context after setup has returned. + const server = createServer((_request, response) => { + const request = new IncomingMessage(new Socket()) + request.url = '/products/test' + request.method = 'GET' + logRequestLine(createEvent(request, new ServerResponse(request)), {status: 200, headers: new Headers()}, { + type: 'theme', + } as Parameters[2]) + response.end('OK') + }) + try { + await new Promise((resolve, reject) => { + server.once('error', reject) + server.listen(0, '127.0.0.1', resolve) + }) + const {port} = server.address() as AddressInfo + await expect((await fetch(`http://127.0.0.1:${port}`)).text()).resolves.toBe('OK') + } finally { + server.closeAllConnections() + await new Promise((resolve) => server.close(() => resolve())) + } + }) + outputInfo('Outside the dev session') + }) + + const log = await readFile(logFile) + const messages = log + .trim() + .split('\n') + .map((line) => JSON.parse(line).message) + .join('\n') + for (const text of [ + 'Synced » update assets/base.css', + 'A dev warning', + 'Invalid CSS', + 'Uploading theme files', + '/products/test', + ]) { + expect(messages).toContain(text) + expect(stderr()).toContain(text) + } + expect(messages).not.toContain('Outside the dev session') + expect(log).not.toContain('\u001b') + expect(log).not.toContain('sessionId') + }) + }) +}) + +test('drains a burst of messages and restores terminal output after failure', async () => { + await inTemporaryDirectory(async (directory) => { + await withCapturedStandardStreams(async ({stderr}) => { + // eslint-disable-next-line no-console + const originalWarn = console.warn + let logFile = '' + await expect( + withDevInstance({command: 'theme dev', directory}, async (instance) => { + logFile = instance.paths.logFile + await withThemeDevOutput(instance, async () => { + for (let index = 0; index < 1000; index++) outputInfo(`message ${index}`) + throw new Error('Setup failed') + }) + }), + ).rejects.toThrow('Setup failed') + // eslint-disable-next-line no-console + expect(console.warn).toBe(originalWarn) + const records = (await readFile(logFile)) + .trim() + .split('\n') + .map((line) => JSON.parse(line)) + expect(records.map(({message}) => message)).toEqual(Array.from({length: 1000}, (_, index) => `message ${index}`)) + expect(stderr()).toContain('message 999') + }) + }) +}) diff --git a/packages/theme/src/cli/services/dev/output.ts b/packages/theme/src/cli/services/dev/output.ts new file mode 100644 index 00000000000..fc1fd43ac59 --- /dev/null +++ b/packages/theme/src/cli/services/dev/output.ts @@ -0,0 +1,44 @@ +/* eslint-disable no-console -- Theme output helpers write messages through console.warn. */ +import {renderCommandEvent, runWithCommandEvents} from '@shopify/cli-kit/node/command-events' +import {type DevInstance} from '@shopify/cli-kit/node/dev-instance' +import {AsyncLocalStorage} from 'node:async_hooks' +import {Writable} from 'node:stream' +import {format} from 'node:util' + +const themeDevOutput = new AsyncLocalStorage() + +export async function withThemeDevOutput(instance: DevInstance, run: () => Promise): Promise { + const output = instance.tee(new Writable({write: (_chunk, _encoding, next) => next()}), { + source: 'theme dev', + stringWritesAreMessages: true, + }) + const originalWarn = console.warn + let active = true + // Ink writes progress frames directly to stderr. Capture messages before presentation, + // and record task events separately so screen redraws do not enter the session log. + console.warn = (...args: unknown[]) => { + if (active && themeDevOutput.getStore() === output) output.write(format(...args)) + originalWarn(...args) + } + try { + return await themeDevOutput.run(output, () => + runWithCommandEvents( + { + outputMode: 'text', + sink: (event, options) => { + if (active && event.type === 'progress' && event.message) output.write(event.message) + if (!options?.alreadyRendered) renderCommandEvent(event) + }, + }, + run, + ), + ) + } finally { + active = false + restoreConsoleWarn() + } + + function restoreConsoleWarn() { + console.warn = originalWarn + } +} diff --git a/packages/theme/src/cli/utilities/theme-environment/dev-server-session.test.ts b/packages/theme/src/cli/utilities/theme-environment/dev-server-session.test.ts index 17d8d65417a..01bcc42243e 100644 --- a/packages/theme/src/cli/utilities/theme-environment/dev-server-session.test.ts +++ b/packages/theme/src/cli/utilities/theme-environment/dev-server-session.test.ts @@ -226,6 +226,25 @@ describe('dev server session', async () => { ) }) + test('clears the session refresh timer on shutdown', async () => { + vi.useFakeTimers() + try { + vi.mocked(ensureAuthenticatedStorefront).mockResolvedValue('storefront_token') + vi.mocked(getStorefrontSessionCookies).mockResolvedValue({_shopify_essential: 'cookie'}) + vi.mocked(ensureAuthenticatedThemes).mockResolvedValue(adminSession) + const controller = new AbortController() + const timerCount = vi.getTimerCount() + await initializeDevServerSession(themeId, adminSession, undefined, undefined, undefined, controller.signal) + expect(vi.getTimerCount()).toBe(timerCount + 1) + controller.abort() + expect(vi.getTimerCount()).toBe(timerCount) + await vi.advanceTimersByTimeAsync(30 * 60 * 1000) + expect(ensureAuthenticatedThemes).toHaveBeenCalledOnce() + } finally { + vi.useRealTimers() + } + }) + test('returns a refreshable session', async () => { // Given for (const index of [1, 2, 3]) { diff --git a/packages/theme/src/cli/utilities/theme-environment/dev-server-session.ts b/packages/theme/src/cli/utilities/theme-environment/dev-server-session.ts index e60705a24df..763c19f36a4 100644 --- a/packages/theme/src/cli/utilities/theme-environment/dev-server-session.ts +++ b/packages/theme/src/cli/utilities/theme-environment/dev-server-session.ts @@ -21,6 +21,7 @@ const REQUIRED_THEME_FILES = ['layout/theme.liquid', 'config/settings_schema.jso * @param adminPassword - Custom app password or password generated by the Theme Access app. * @param storefrontPassword - Storefront password set in password-protected stores. * @param crawlerSignatureHeaders - Crawler signature headers sent with storefront requests. + * @param signal - Stops automatic refresh when the dev session ends. * * @returns Details about the app configuration state. */ @@ -30,6 +31,7 @@ export async function initializeDevServerSession( adminPassword?: string, storefrontPassword?: string, crawlerSignatureHeaders?: CrawlerSignatureHeaders, + signal?: AbortSignal, ) { const session = await fetchDevServerSession( themeId, @@ -53,7 +55,8 @@ export async function initializeDevServerSession( return newSession } - setInterval(() => { + if (signal?.aborted) return session + const refreshInterval = setInterval(() => { if (!session.refresh) return session @@ -61,6 +64,7 @@ export async function initializeDevServerSession( .then(() => outputDebug('Refreshed theme session via auto-refresher...')) .catch(() => outputDebug('Session could not be refreshed.')) }, SESSION_TIMEOUT_IN_MS) + signal?.addEventListener('abort', () => clearInterval(refreshInterval), {once: true}) return session } 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 780534e0e42..137e0302441 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 @@ -23,6 +23,7 @@ export async function reconcileAndPollThemeEditorChanges( ignore: string[] only: string[] reconciliationStrategy?: ReconciliationStrategy + signal?: AbortSignal }, rejectBackgroundJob: (reason?: unknown) => void, ): Promise<{ @@ -36,7 +37,9 @@ export async function reconcileAndPollThemeEditorChanges( const updatedRemoteChecksumsPromise = workPromise.then(async () => { const updatedRemoteChecksums = await fetchChecksums(targetTheme.id, session) - pollThemeEditorChanges( + // Polling handles its own errors and runs until cancellation. + // eslint-disable-next-line no-void + void pollThemeEditorChanges( targetTheme, session, updatedRemoteChecksums, 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 f7fe307e700..c42eb01ffbe 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 @@ -18,6 +18,7 @@ import {buildTheme} from '@shopify/cli-kit/node/themes/factories' import {createEvent} from 'h3' import * as output from '@shopify/cli-kit/node/output' import {fetchChecksums} from '@shopify/cli-kit/node/themes/api' +import {getAvailableTCPPort, checkPortAvailability} from '@shopify/cli-kit/node/tcp' import {IncomingMessage, ServerResponse} from 'node:http' import {Socket} from 'node:net' @@ -176,6 +177,26 @@ describe('setupDevServer', () => { localThemeFileSystem.unsyncedFileKeys.clear() }) + test('closes the listening server and releases its port when the dev session stops', async () => { + const controller = new AbortController() + const port = await getAvailableTCPPort() + const server = setupDevServer(developmentTheme, { + ...defaultServerContext, + signal: controller.signal, + options: {...defaultServerContext.options, host: 'localhost', port}, + }) + try { + await server.workPromise + await server.serverStart() + await expect(checkPortAvailability(port)).resolves.toBe(false) + controller.abort() + } finally { + controller.abort() + await server.serverStop() + } + await expect(checkPortAvailability(port)).resolves.toBe(true) + }) + test('should upload the development theme to remote', async () => { // Given const context: DevServerContext = { 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 f5294e1f912..d8d0e78305e 100644 --- a/packages/theme/src/cli/utilities/theme-environment/theme-environment.ts +++ b/packages/theme/src/cli/utilities/theme-environment/theme-environment.ts @@ -34,6 +34,7 @@ export function setupDevServer(theme: Theme, ctx: DevServerContext) { return { workPromise, serverStart: server.start, + serverStop: server.close, dispatchEvent: server.dispatch, renderDevSetupProgress: envSetup.renderProgress, backgroundJobPromise, @@ -47,7 +48,7 @@ function ensureThemeEnvironmentSetup( rejectBackgroundJob: (reason?: unknown) => void, ) { const abort = (error: Error): never => { - renderThrownError('Failed to perform the initial theme synchronization.', error) + if (!ctx.signal?.aborted) renderThrownError('Failed to perform the initial theme synchronization.', error) rejectBackgroundJob(error) // Return a never-resolving promise to stop this promise chain without throwing. // Throwing would trigger catch handlers and continue execution. This stops the @@ -57,12 +58,14 @@ function ensureThemeEnvironmentSetup( const remoteChecksumsPromise = fetchChecksums(theme.id, ctx.session) - const reconcilePromise = remoteChecksumsPromise.then((remoteChecksums) => - handleThemeEditorSync(theme, ctx, remoteChecksums, rejectBackgroundJob), - ) + const reconcilePromise = remoteChecksumsPromise.then((remoteChecksums) => { + ctx.signal?.throwIfAborted() + return handleThemeEditorSync(theme, ctx, remoteChecksums, rejectBackgroundJob) + }) const uploadPromise = reconcilePromise.then(async ({updatedRemoteChecksumsPromise}) => { const updatedRemoteChecksums = await updatedRemoteChecksumsPromise + ctx.signal?.throwIfAborted() return uploadTheme(theme, ctx.session, updatedRemoteChecksums, ctx.localThemeFileSystem, { nodelete: ctx.options.noDelete, deferPartialWork: true, @@ -114,6 +117,7 @@ function handleThemeEditorSync( ignore: ctx.options.ignore, only: ctx.options.only, reconciliationStrategy: ctx.options.reconciliationStrategy, + signal: ctx.signal, }, rejectBackgroundJob, ) @@ -170,23 +174,22 @@ function createDevelopmentServer(theme: Theme, ctx: DevServerContext, initialWor const server = createServer(toNodeListener(app)) + const close = async () => { + server.closeAllConnections() + await new Promise((resolve) => server.close(() => resolve())) + } return { dispatch: app.handler.bind(app), + close, start: async (): Promise => { - return new Promise((resolve) => - server.listen({port: ctx.options.port, host: ctx.options.host}, () => - resolve({ - close: async () => { - await Promise.all([ - new Promise((resolve) => { - server.closeAllConnections() - server.close(resolve) - }), - ]) - }, - }), - ), - ) + ctx.signal?.throwIfAborted() + return new Promise((resolve, reject) => { + server.once('error', reject) + server.listen({port: ctx.options.port, host: ctx.options.host, signal: ctx.signal}, () => { + server.off('error', reject) + resolve({close}) + }) + }) }, } } diff --git a/packages/theme/src/cli/utilities/theme-environment/theme-polling.test.ts b/packages/theme/src/cli/utilities/theme-environment/theme-polling.test.ts index 479338388f8..edbc221edc7 100644 --- a/packages/theme/src/cli/utilities/theme-environment/theme-polling.test.ts +++ b/packages/theme/src/cli/utilities/theme-environment/theme-polling.test.ts @@ -1,4 +1,4 @@ -import {PollingOptions, pollRemoteJsonChanges, deleteRemovedAssets} from './theme-polling.js' +import {PollingOptions, pollRemoteJsonChanges, deleteRemovedAssets, pollThemeEditorChanges} from './theme-polling.js' import {fakeThemeFileSystem} from '../theme-fs/theme-fs-mock-factory.js' import {fetchChecksums, fetchThemeAssets} from '@shopify/cli-kit/node/themes/api' import {Checksum, ThemeAsset} from '@shopify/cli-kit/node/themes/types' @@ -319,3 +319,20 @@ describe('pollRemoteJsonChanges', async () => { }) }) }) + +test('cancels the polling timer when the dev session stops', async () => { + const controller = new AbortController() + const onError = vi.fn() + const polling = pollThemeEditorChanges( + buildTheme({id: 1, name: 'Theme', role: DEVELOPMENT_THEME_ROLE})!, + {token: '', storeFqdn: ''}, + [], + fakeThemeFileSystem('tmp', new Map()), + {noDelete: false, signal: controller.signal}, + onError, + ) + controller.abort() + await polling + expect(fetchChecksums).not.toHaveBeenCalled() + expect(onError).not.toHaveBeenCalled() +}) diff --git a/packages/theme/src/cli/utilities/theme-environment/theme-polling.ts b/packages/theme/src/cli/utilities/theme-environment/theme-polling.ts index 03c325abdcc..189aeaf4354 100644 --- a/packages/theme/src/cli/utilities/theme-environment/theme-polling.ts +++ b/packages/theme/src/cli/utilities/theme-environment/theme-polling.ts @@ -7,12 +7,14 @@ import {outputDebug, outputInfo, outputContent, outputToken} from '@shopify/cli- import {AdminSession} from '@shopify/cli-kit/node/session' import {renderFatalError} from '@shopify/cli-kit/node/ui' import {AbortError} from '@shopify/cli-kit/node/error' +import {setTimeout} from 'node:timers/promises' const POLLING_INTERVAL = 3000 class PollingError extends Error {} export interface PollingOptions { noDelete: boolean + signal?: AbortSignal } export function pollThemeEditorChanges( @@ -33,7 +35,8 @@ export function pollThemeEditorChanges( const poll = async () => { // Asynchronously wait for the polling interval, similar to a setInterval // but ensure the polling work is done before starting the next interval. - await new Promise((resolve) => setTimeout(resolve, POLLING_INTERVAL)) + await setTimeout(POLLING_INTERVAL, undefined, {signal: options.signal}) + if (options.signal?.aborted) return // eslint-disable-next-line require-atomic-updates latestChecksums = await pollRemoteJsonChanges(targetTheme, session, latestChecksums, localFileSystem, options) @@ -64,12 +67,15 @@ export function pollThemeEditorChanges( }) } - // eslint-disable-next-line @typescript-eslint/no-misused-promises - setTimeout(async () => { - while (true) { + const run = async () => { + while (!options.signal?.aborted) { // eslint-disable-next-line no-await-in-loop await poll() + if (failedPollingAttempts >= maxPollingAttempts) break } + } + return run().catch((error: unknown) => { + if (!options.signal?.aborted) rejectBackgroundJob(error) }) } diff --git a/packages/theme/src/cli/utilities/theme-environment/types.ts b/packages/theme/src/cli/utilities/theme-environment/types.ts index 3ba820d1a24..7abcad7b8d5 100644 --- a/packages/theme/src/cli/utilities/theme-environment/types.ts +++ b/packages/theme/src/cli/utilities/theme-environment/types.ts @@ -73,6 +73,9 @@ export type ReconciliationStrategy = 'keep-local' | 'keep-remote' | 'abort' * Maintains the state of local and remote assets in theme development server. */ export interface DevServerContext { + /** Stops work owned by a theme dev session. */ + signal?: AbortSignal + /** * Authentication session for development server operations. */ diff --git a/packages/theme/src/cli/utilities/theme-fs.test.ts b/packages/theme/src/cli/utilities/theme-fs.test.ts index 66d7c1704f9..4f0c31ce68f 100644 --- a/packages/theme/src/cli/utilities/theme-fs.test.ts +++ b/packages/theme/src/cli/utilities/theme-fs.test.ts @@ -147,6 +147,32 @@ describe('theme-fs', () => { }) }) + test('closes the watcher and cancels pending file changes when the dev session stops', async () => { + await inTemporaryDirectory(async (root) => { + await copyDirectoryContents(joinPath(locationOfThisFile, 'fixtures/theme'), root) + const watcher = Object.assign(new EventEmitter(), {close: vi.fn(async () => {})}) + vi.spyOn(chokidar, 'watch').mockReturnValue(watcher as unknown as ReturnType) + const controller = new AbortController() + const fileSystem = mountThemeFileSystem(root, {signal: controller.signal}) + await fileSystem.ready() + await fileSystem.startWatcher('123', {token: 'token', storeFqdn: 'store.myshopify.com'}) + vi.useFakeTimers() + try { + watcher.emit('change', joinPath(root, 'assets/base.css')) + expect(vi.getTimerCount()).toBe(1) + controller.abort() + expect(watcher.close).toHaveBeenCalledOnce() + expect(vi.getTimerCount()).toBe(0) + watcher.emit('change', joinPath(root, 'assets/base.css')) + await vi.runAllTimersAsync() + expect(bulkUploadThemeAssets).not.toHaveBeenCalled() + } finally { + vi.useRealTimers() + controller.abort() + } + }) + }) + test('includes listing directory in watched directories when listing is specified', async () => { await inTemporaryDirectory(async (tmpDir) => { // Given diff --git a/packages/theme/src/cli/utilities/theme-fs.ts b/packages/theme/src/cli/utilities/theme-fs.ts index 5448fe00ba4..381e97ecf40 100644 --- a/packages/theme/src/cli/utilities/theme-fs.ts +++ b/packages/theme/src/cli/utilities/theme-fs.ts @@ -59,7 +59,10 @@ const THEME_PARTITION_REGEX = { staticAssetRegex: /^assets\/(?!.*\.liquid$)/, } -export function mountThemeFileSystem(root: string, options?: ThemeFileSystemOptions): ThemeFileSystem { +export function mountThemeFileSystem( + root: string, + options?: ThemeFileSystemOptions & {signal?: AbortSignal}, +): ThemeFileSystem { if (options?.listing !== undefined) validateListingName(options.listing) const files = new Map() @@ -329,6 +332,7 @@ export function mountThemeFileSystem(root: string, options?: ThemeFileSystemOpti }, startWatcher: async (themeId: string, adminSession: AdminSession) => { const {default: chokidar} = await import('chokidar') + if (options?.signal?.aborted) return const watcher = chokidar.watch([...directoriesToWatch], { ignored: DEFAULT_IGNORE_PATTERNS, @@ -338,8 +342,16 @@ export function mountThemeFileSystem(root: string, options?: ThemeFileSystemOpti // Debounce file events per-file and per-event-type const pendingEvents = new Map() + const stop = () => { + for (const timeout of pendingEvents.values()) clearTimeout(timeout) + pendingEvents.clear() + watcher.close().catch((error: Error) => outputWarn(`Error closing file watcher: ${error.message}`)) + } + options?.signal?.addEventListener('abort', stop, {once: true}) + watcher.once('close', () => options?.signal?.removeEventListener('abort', stop)) const queueFsEvent = (eventName: 'add' | 'change' | 'unlink', filePath: string) => { + if (options?.signal?.aborted) return const fileKey = getKey(filePath) const eventKey = `${fileKey}:${eventName}` diff --git a/packages/theme/test/fixtures/dev-instance-dependencies.js b/packages/theme/test/fixtures/dev-instance-dependencies.js new file mode 100644 index 00000000000..5872bef80ff --- /dev/null +++ b/packages/theme/test/fixtures/dev-instance-dependencies.js @@ -0,0 +1,43 @@ +import {setImmediate} from 'node:timers/promises' + +import {readDevInstance} from '@shopify/cli-kit/node/dev-instance' +import {outputInfo} from '@shopify/cli-kit/node/output' + +export const hasRequiredThemeDirectories = async () => true +export const ensureDirectoryConfirmed = async () => true +export const mountThemeFileSystem = () => ({}) +export const emptyThemeExtFileSystem = () => ({}) +export const isStorefrontPasswordProtected = async () => false +export const ensureValidPassword = async () => undefined +export const initializeDevServerSession = async () => ({storeFqdn: 'fixture.myshopify.com', token: 'fixture'}) +export const fetchOrCreateCrawlerSignatureHeaders = async () => undefined +export const ensureListingExists = async () => {} + +export function setupDevServer(theme, ctx) { + return { + workPromise: Promise.resolve(), + renderDevSetupProgress: async () => {}, + backgroundJobPromise: new Promise(() => {}), + serverStart: async () => { + // Keep a handle alive, as the real HTTP server and watchers do. + setInterval(() => {}, 1000) + const startupDeadline = setTimeout(() => process.exit(2), 20000) + const sendSignal = async () => { + const identity = {command: 'theme dev', directory: ctx.directory, key: `${ctx.session.storeFqdn}:${theme.id}`} + while ((await readDevInstance(identity))?.status !== 'running') await setImmediate() + clearTimeout(startupDeadline) + for (let index = 0; index < 1000; index++) outputInfo(`line ${index}`) + setTimeout(() => process.exit(2), 20000) + process.kill(process.pid, process.env.THEME_DEV_TEST_SIGNAL) + } + sendSignal().catch((error) => { + process.stderr.write(String(error)) + process.exit(2) + }) + }, + serverStop: async () => { + if (!ctx.signal.aborted) throw new Error('Server stopped before cancellation') + outputInfo('Stopped') + }, + } +} diff --git a/packages/theme/test/fixtures/dev-instance-loader.js b/packages/theme/test/fixtures/dev-instance-loader.js new file mode 100644 index 00000000000..172832a9374 --- /dev/null +++ b/packages/theme/test/fixtures/dev-instance-loader.js @@ -0,0 +1,22 @@ +import {resolve as resolveCliKitSource} from '../../../cli-kit/test/fixtures/cli-kit-source-loader.js' + +const serviceUrl = new URL('../../src/cli/services/dev.ts', import.meta.url).href +const dependencies = new Set([ + '../utilities/theme-fs.js', + '../utilities/theme-ui.js', + '../utilities/theme-environment/theme-environment.js', + '../utilities/theme-environment/storefront-session.js', + '../utilities/theme-environment/storefront-password-prompt.js', + '../utilities/theme-fs-empty.js', + '../utilities/theme-environment/dev-server-session.js', + '../utilities/theme-environment/crawler-signature.js', + '../utilities/theme-listing.js', +]) + +// Use the real service and CLI Kit sources; replace only remote theme setup. +export function resolve(specifier, context, nextResolve) { + if (context.parentURL === serviceUrl && dependencies.has(specifier)) { + return nextResolve(new URL('./dev-instance-dependencies.js', import.meta.url).href, context) + } + return resolveCliKitSource(specifier, context, nextResolve) +} From 9e99e3bc8b6ff1bb61fd400263f623322a3e5113 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Wed, 21 Oct 2026 18:20:08 +0200 Subject: [PATCH 2/4] Omit redundant sources from theme dev logs --- .changeset/optional-dev-log-source.md | 5 +++ docs/cli/theme-dev-agent-workflow.md | 36 ------------------- .../src/private/node/dev-instance-output.ts | 9 +++-- .../src/public/node/dev-instance.test.ts | 20 +++++++++++ .../cli-kit/src/public/node/dev-instance.ts | 3 +- packages/cli/README.md | 3 +- packages/cli/oclif.manifest.json | 2 +- .../theme/src/cli/services/dev/output.test.ts | 1 + packages/theme/src/cli/services/dev/output.ts | 1 - 9 files changed, 36 insertions(+), 44 deletions(-) create mode 100644 .changeset/optional-dev-log-source.md delete mode 100644 docs/cli/theme-dev-agent-workflow.md diff --git a/.changeset/optional-dev-log-source.md b/.changeset/optional-dev-log-source.md new file mode 100644 index 00000000000..587eb3d5c51 --- /dev/null +++ b/.changeset/optional-dev-log-source.md @@ -0,0 +1,5 @@ +--- +'@shopify/cli-kit': minor +--- + +Allow Dev Instance logs to omit the source field when all messages have the same producer. diff --git a/docs/cli/theme-dev-agent-workflow.md b/docs/cli/theme-dev-agent-workflow.md deleted file mode 100644 index 412727909d0..00000000000 --- a/docs/cli/theme-dev-agent-workflow.md +++ /dev/null @@ -1,36 +0,0 @@ -# Theme dev session files - -`shopify theme dev` writes instance metadata and a JSONL log under -`/.shopify/dev/`. Agents can read these files to find a session started in -another terminal. No IPC endpoint or extra command is required. - -Each instance uses the store and theme ID as its key. Two sessions for the same -store and theme in the same directory cannot run together, including sessions -with different listings. Different stores or theme IDs can run together. - -The instance JSON includes the PID, `sessionId`, `logFile`, and `status`: - -- `starting`: the CLI is preparing the session or synchronizing the theme. -- `running`: the server is listening and initial synchronization has completed. - -`details` contains `store`, `themeId`, `themeName`, `listing` when set, `host`, -`port`, and `urls` with `local`, `preview`, `themeEditor`, and `giftCard` links. -The URLs are added during setup. `running` is not a health check or proof that -later edits have reached the store. Read new output and test the result. - -The log contains theme dev messages, file synchronization results, request lines, -errors, and setup progress. Each complete line contains `timestamp`, `source`, -and `message`. Colors and progress redraws are excluded. Authentication before -the dev session and fatal CLI errors reported after it are outside this log. - -Read the instance JSON's `logFile` field to find the log. Keep a byte offset to -read new complete lines. Reset the offset if `sessionId` changes: each new run -clears the log. Logs have no size limit or rotation. - -Before starting another session, check the PID and lock heartbeat. If a second -start reports `dev_instance_already_running`, use the reported session files. -Do not stop a session started by the developer. The shared lock recovery and -interrupted-start cleanup rules are described in [App dev session files](app-dev-agent-workflow.md). - -Ctrl+C and SIGTERM stop dev work and drain queued log messages before exit. -Normal shutdown removes the instance JSON and lock, and retains the log. diff --git a/packages/cli-kit/src/private/node/dev-instance-output.ts b/packages/cli-kit/src/private/node/dev-instance-output.ts index 44f069c60a3..ea98ca3306e 100644 --- a/packages/cli-kit/src/private/node/dev-instance-output.ts +++ b/packages/cli-kit/src/private/node/dev-instance-output.ts @@ -17,7 +17,10 @@ export const devInstanceLogOutputSchema = defineJsonOutputSchema({ schema: zod .object({ timestamp: jsonOutputTimestampSchema, - source: zod.string().describe('What produced the line, for example a process prefix like web or extensions.'), + source: zod + .string() + .optional() + .describe('What produced the line, for example a process prefix like web or extensions, when needed.'), level: zod.enum(['debug', 'info', 'warning', 'error']).optional(), message: zod.string(), }) @@ -63,7 +66,7 @@ export function createDevInstanceOutput({logFile, logOutput, isActive, onError}: log: writeLogEntry, tee: ( target: Writable, - {source, stringWritesAreMessages = false}: {source: string; stringWritesAreMessages?: boolean}, + {source, stringWritesAreMessages = false}: {source?: string; stringWritesAreMessages?: boolean}, ) => { const decoder = new StringDecoder('utf8') let pending = '' @@ -82,7 +85,7 @@ export function createDevInstanceOutput({logFile, logOutput, isActive, onError}: const output = new DevInstanceOutput({ decodeStrings: !stringWritesAreMessages, write(chunk: Buffer | string, encoding, callback) { - const nextSource = getConcurrentOutputContext()?.outputPrefix ?? source + const nextSource = source === undefined ? undefined : (getConcurrentOutputContext()?.outputPrefix ?? source) const completeMessage = stringWritesAreMessages && typeof chunk === 'string' const previous = nextSource !== currentSource || completeMessage ? flush() : Promise.resolve() currentSource = nextSource diff --git a/packages/cli-kit/src/public/node/dev-instance.test.ts b/packages/cli-kit/src/public/node/dev-instance.test.ts index 066890ab9c5..1f980fc27b4 100644 --- a/packages/cli-kit/src/public/node/dev-instance.test.ts +++ b/packages/cli-kit/src/public/node/dev-instance.test.ts @@ -69,6 +69,26 @@ describe('withDevInstance', () => { }) describe('acquireDevInstance', () => { + test('omits sources from direct messages and streams when no source is supplied', async () => { + await inTemporaryDirectory(async (directory) => { + const mirror = collectingStream() + const target = collectingStream() + const identity = {command: 'theme dev', directory} + await withDevInstance({...identity, logOutput: mirror.stream}, async (instance) => { + await instance.log({message: 'Setup'}) + const output = instance.tee(target.stream, {stringWritesAreMessages: true}) + useConcurrentOutputContext({outputPrefix: 'theme'}, () => output.write('Ready\n')) + }) + const {logFile} = devInstancePaths(identity) + await expect(readLog(logFile)).resolves.toEqual([ + {timestamp: expect.any(String), message: 'Setup'}, + {timestamp: expect.any(String), message: 'Ready'}, + ]) + expect(target.chunks.join('')).toBe('Ready\n') + expect(mirror.chunks.join('')).toBe(await readFile(logFile)) + }) + }) + test('does not include CLI fatal errors in the session log schema', () => { expect(() => devInstanceLogOutputSchema.validate({ diff --git a/packages/cli-kit/src/public/node/dev-instance.ts b/packages/cli-kit/src/public/node/dev-instance.ts index b1bbca1663c..40e53a4e102 100644 --- a/packages/cli-kit/src/public/node/dev-instance.ts +++ b/packages/cli-kit/src/public/node/dev-instance.ts @@ -95,7 +95,8 @@ export interface DevInstance { tee: ( target: Writable, options: { - source: string + /** Omit the source field when the stream does not need to distinguish producers. */ + source?: string /** Treat each string write as a complete message. Byte writes remain buffered into lines. */ stringWritesAreMessages?: boolean }, diff --git a/packages/cli/README.md b/packages/cli/README.md index c59bf75e11d..a3da2d36a6a 100644 --- a/packages/cli/README.md +++ b/packages/cli/README.md @@ -2009,7 +2009,7 @@ DESCRIPTION }, "source": { "type": "string", - "description": "What produced the line, for example a process prefix like web or extensions." + "description": "What produced the line, for example a process prefix like web or extensions, when needed." }, "level": { "type": "string", @@ -2026,7 +2026,6 @@ DESCRIPTION }, "required": [ "timestamp", - "source", "message" ], "additionalProperties": false, diff --git a/packages/cli/oclif.manifest.json b/packages/cli/oclif.manifest.json index d9833c0a556..1c8e81bed44 100644 --- a/packages/cli/oclif.manifest.json +++ b/packages/cli/oclif.manifest.json @@ -1191,7 +1191,7 @@ "args": { }, "customPluginName": "@shopify/app", - "description": "Builds and previews your app on a dev store, and watches for changes. \"Read more about testing apps locally\" (https://shopify.dev/docs/apps/build/cli-for-apps/test-apps-locally).\n\nUse `--json-schema` to print the result, error, and event schemas.\n\nOutput from `--json` conforms to the `DevInstanceLogEntry` schema.\n\n```json\n{\n \"type\": \"object\",\n \"properties\": {\n \"timestamp\": {\n \"$ref\": \"#/definitions/DevInstance/properties/startedAt\"\n },\n \"source\": {\n \"type\": \"string\",\n \"description\": \"What produced the line, for example a process prefix like web or extensions.\"\n },\n \"level\": {\n \"type\": \"string\",\n \"enum\": [\n \"debug\",\n \"info\",\n \"warning\",\n \"error\"\n ]\n },\n \"message\": {\n \"type\": \"string\"\n }\n },\n \"required\": [\n \"timestamp\",\n \"source\",\n \"message\"\n ],\n \"additionalProperties\": false,\n \"title\": \"DevInstanceLogEntry\",\n \"definitions\": {\n \"DevInstance\": {\n \"type\": \"object\",\n \"properties\": {\n \"schemaVersion\": {\n \"type\": \"number\",\n \"const\": 1\n },\n \"sessionId\": {\n \"type\": \"string\"\n },\n \"command\": {\n \"type\": \"string\",\n \"const\": \"app dev\"\n },\n \"key\": {\n \"type\": \"string\"\n },\n \"pid\": {\n \"type\": \"integer\",\n \"exclusiveMinimum\": 0\n },\n \"status\": {\n \"type\": \"string\",\n \"enum\": [\n \"starting\",\n \"running\"\n ]\n },\n \"startedAt\": {\n \"type\": \"string\",\n \"format\": \"date-time\",\n \"pattern\": \"^\\\\d{4}-\\\\d{2}-\\\\d{2}T\\\\d{2}:\\\\d{2}:\\\\d{2}Z$\",\n \"description\": \"A UTC ISO 8601 instant with whole seconds and the Z timezone marker.\"\n },\n \"updatedAt\": {\n \"$ref\": \"#/definitions/DevInstance/properties/startedAt\"\n },\n \"directory\": {\n \"type\": \"string\"\n },\n \"logFile\": {\n \"type\": \"string\"\n },\n \"details\": {\n \"type\": \"object\",\n \"properties\": {\n \"configPath\": {\n \"type\": \"string\"\n },\n \"appName\": {\n \"type\": \"string\"\n },\n \"clientId\": {\n \"type\": \"string\"\n },\n \"storeFqdn\": {\n \"type\": \"string\"\n },\n \"previewUrl\": {\n \"type\": \"string\"\n },\n \"graphiqlUrl\": {\n \"type\": \"string\"\n },\n \"appUrl\": {\n \"type\": \"string\"\n },\n \"localUrl\": {\n \"type\": \"string\"\n },\n \"preview\": {\n \"$ref\": \"#/definitions/DevPreview\",\n \"description\": \"The CLI-reported preview state in the instance file; this is not a health check.\"\n }\n },\n \"required\": [\n \"configPath\",\n \"appName\",\n \"clientId\"\n ],\n \"additionalProperties\": false\n }\n },\n \"required\": [\n \"schemaVersion\",\n \"sessionId\",\n \"command\",\n \"pid\",\n \"status\",\n \"startedAt\",\n \"updatedAt\",\n \"directory\",\n \"logFile\",\n \"details\"\n ],\n \"additionalProperties\": false,\n \"description\": \"The instance JSON file under .shopify/dev/. This object is not emitted to stdout.\"\n },\n \"DevPreview\": {\n \"type\": \"object\",\n \"properties\": {\n \"state\": {\n \"type\": \"string\",\n \"enum\": [\n \"starting\",\n \"updating\",\n \"ready\",\n \"error\"\n ]\n },\n \"message\": {\n \"type\": [\n \"string\",\n \"null\"\n ]\n },\n \"errors\": {\n \"type\": \"array\",\n \"items\": {\n \"type\": \"string\"\n }\n }\n },\n \"required\": [\n \"state\",\n \"message\",\n \"errors\"\n ],\n \"additionalProperties\": false,\n \"description\": \"The CLI-reported preview state in the instance file; this is not a health check.\"\n }\n },\n \"$schema\": \"http://json-schema.org/draft-07/schema#\"\n}\n```", + "description": "Builds and previews your app on a dev store, and watches for changes. \"Read more about testing apps locally\" (https://shopify.dev/docs/apps/build/cli-for-apps/test-apps-locally).\n\nUse `--json-schema` to print the result, error, and event schemas.\n\nOutput from `--json` conforms to the `DevInstanceLogEntry` schema.\n\n```json\n{\n \"type\": \"object\",\n \"properties\": {\n \"timestamp\": {\n \"$ref\": \"#/definitions/DevInstance/properties/startedAt\"\n },\n \"source\": {\n \"type\": \"string\",\n \"description\": \"What produced the line, for example a process prefix like web or extensions, when needed.\"\n },\n \"level\": {\n \"type\": \"string\",\n \"enum\": [\n \"debug\",\n \"info\",\n \"warning\",\n \"error\"\n ]\n },\n \"message\": {\n \"type\": \"string\"\n }\n },\n \"required\": [\n \"timestamp\",\n \"message\"\n ],\n \"additionalProperties\": false,\n \"title\": \"DevInstanceLogEntry\",\n \"definitions\": {\n \"DevInstance\": {\n \"type\": \"object\",\n \"properties\": {\n \"schemaVersion\": {\n \"type\": \"number\",\n \"const\": 1\n },\n \"sessionId\": {\n \"type\": \"string\"\n },\n \"command\": {\n \"type\": \"string\",\n \"const\": \"app dev\"\n },\n \"key\": {\n \"type\": \"string\"\n },\n \"pid\": {\n \"type\": \"integer\",\n \"exclusiveMinimum\": 0\n },\n \"status\": {\n \"type\": \"string\",\n \"enum\": [\n \"starting\",\n \"running\"\n ]\n },\n \"startedAt\": {\n \"type\": \"string\",\n \"format\": \"date-time\",\n \"pattern\": \"^\\\\d{4}-\\\\d{2}-\\\\d{2}T\\\\d{2}:\\\\d{2}:\\\\d{2}Z$\",\n \"description\": \"A UTC ISO 8601 instant with whole seconds and the Z timezone marker.\"\n },\n \"updatedAt\": {\n \"$ref\": \"#/definitions/DevInstance/properties/startedAt\"\n },\n \"directory\": {\n \"type\": \"string\"\n },\n \"logFile\": {\n \"type\": \"string\"\n },\n \"details\": {\n \"type\": \"object\",\n \"properties\": {\n \"configPath\": {\n \"type\": \"string\"\n },\n \"appName\": {\n \"type\": \"string\"\n },\n \"clientId\": {\n \"type\": \"string\"\n },\n \"storeFqdn\": {\n \"type\": \"string\"\n },\n \"previewUrl\": {\n \"type\": \"string\"\n },\n \"graphiqlUrl\": {\n \"type\": \"string\"\n },\n \"appUrl\": {\n \"type\": \"string\"\n },\n \"localUrl\": {\n \"type\": \"string\"\n },\n \"preview\": {\n \"$ref\": \"#/definitions/DevPreview\",\n \"description\": \"The CLI-reported preview state in the instance file; this is not a health check.\"\n }\n },\n \"required\": [\n \"configPath\",\n \"appName\",\n \"clientId\"\n ],\n \"additionalProperties\": false\n }\n },\n \"required\": [\n \"schemaVersion\",\n \"sessionId\",\n \"command\",\n \"pid\",\n \"status\",\n \"startedAt\",\n \"updatedAt\",\n \"directory\",\n \"logFile\",\n \"details\"\n ],\n \"additionalProperties\": false,\n \"description\": \"The instance JSON file under .shopify/dev/. This object is not emitted to stdout.\"\n },\n \"DevPreview\": {\n \"type\": \"object\",\n \"properties\": {\n \"state\": {\n \"type\": \"string\",\n \"enum\": [\n \"starting\",\n \"updating\",\n \"ready\",\n \"error\"\n ]\n },\n \"message\": {\n \"type\": [\n \"string\",\n \"null\"\n ]\n },\n \"errors\": {\n \"type\": \"array\",\n \"items\": {\n \"type\": \"string\"\n }\n }\n },\n \"required\": [\n \"state\",\n \"message\",\n \"errors\"\n ],\n \"additionalProperties\": false,\n \"description\": \"The CLI-reported preview state in the instance file; this is not a health check.\"\n }\n },\n \"$schema\": \"http://json-schema.org/draft-07/schema#\"\n}\n```", "descriptionWithMarkdown": "Builds and previews your app on a dev store, and watches for changes. [Read more about testing apps locally](https://shopify.dev/docs/apps/build/cli-for-apps/test-apps-locally).", "flags": { "auth-alias": { diff --git a/packages/theme/src/cli/services/dev/output.test.ts b/packages/theme/src/cli/services/dev/output.test.ts index 8a5462c1da6..759ad3ed4cc 100644 --- a/packages/theme/src/cli/services/dev/output.test.ts +++ b/packages/theme/src/cli/services/dev/output.test.ts @@ -70,6 +70,7 @@ test('records theme messages, task progress, sync errors, and late HTTP request expect(messages).not.toContain('Outside the dev session') expect(log).not.toContain('\u001b') expect(log).not.toContain('sessionId') + expect(log).not.toContain('"source"') }) }) }) diff --git a/packages/theme/src/cli/services/dev/output.ts b/packages/theme/src/cli/services/dev/output.ts index fc1fd43ac59..fcd930030bd 100644 --- a/packages/theme/src/cli/services/dev/output.ts +++ b/packages/theme/src/cli/services/dev/output.ts @@ -9,7 +9,6 @@ const themeDevOutput = new AsyncLocalStorage() export async function withThemeDevOutput(instance: DevInstance, run: () => Promise): Promise { const output = instance.tee(new Writable({write: (_chunk, _encoding, next) => next()}), { - source: 'theme dev', stringWritesAreMessages: true, }) const originalWarn = console.warn From a08dd468cc311b0a984e934120cd9df1fbf0e6a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Thu, 22 Oct 2026 16:11:40 +0200 Subject: [PATCH 3/4] Check progress logs without requiring a terminal redraw --- packages/theme/src/cli/services/dev/output.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/theme/src/cli/services/dev/output.test.ts b/packages/theme/src/cli/services/dev/output.test.ts index 759ad3ed4cc..2ceefffff4e 100644 --- a/packages/theme/src/cli/services/dev/output.test.ts +++ b/packages/theme/src/cli/services/dev/output.test.ts @@ -61,12 +61,12 @@ test('records theme messages, task progress, sync errors, and late HTTP request 'Synced » update assets/base.css', 'A dev warning', 'Invalid CSS', - 'Uploading theme files', '/products/test', ]) { expect(messages).toContain(text) expect(stderr()).toContain(text) } + expect(messages).toContain('Uploading theme files') expect(messages).not.toContain('Outside the dev session') expect(log).not.toContain('\u001b') expect(log).not.toContain('sessionId') From ef7d27abe719533e82bfdea75e49c5bf695bb330 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Thu, 22 Oct 2026 16:20:33 +0200 Subject: [PATCH 4/4] Format the theme output assertions --- packages/theme/src/cli/services/dev/output.test.ts | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/packages/theme/src/cli/services/dev/output.test.ts b/packages/theme/src/cli/services/dev/output.test.ts index 2ceefffff4e..e325108fea4 100644 --- a/packages/theme/src/cli/services/dev/output.test.ts +++ b/packages/theme/src/cli/services/dev/output.test.ts @@ -57,12 +57,7 @@ test('records theme messages, task progress, sync errors, and late HTTP request .split('\n') .map((line) => JSON.parse(line).message) .join('\n') - for (const text of [ - 'Synced » update assets/base.css', - 'A dev warning', - 'Invalid CSS', - '/products/test', - ]) { + for (const text of ['Synced » update assets/base.css', 'A dev warning', 'Invalid CSS', '/products/test']) { expect(messages).toContain(text) expect(stderr()).toContain(text) }