diff --git a/.changeset/app-dev-instance-lock.md b/.changeset/app-dev-instance-lock.md new file mode 100644 index 00000000000..e4f8610153c --- /dev/null +++ b/.changeset/app-dev-instance-lock.md @@ -0,0 +1,5 @@ +--- +'@shopify/app': minor +--- + +Record `app dev` sessions, preview status, and JSONL logs so agents can reuse a running session. diff --git a/.changeset/windows-dev-shutdown.md b/.changeset/windows-dev-shutdown.md new file mode 100644 index 00000000000..dc618b51e56 --- /dev/null +++ b/.changeset/windows-dev-shutdown.md @@ -0,0 +1,6 @@ +--- +'@shopify/cli-kit': patch +'@shopify/cli': patch +--- + +Preserve graceful shutdown and child-process cleanup on Windows. diff --git a/docs/README.md b/docs/README.md index 34222272eb1..48a88de82a9 100644 --- a/docs/README.md +++ b/docs/README.md @@ -15,6 +15,7 @@ The list below contains valuable resources for people interested in contributing * [Designing for CLI](./cli/designing-for-cli.md) * [Conventions](./cli/conventions.md) * [JSON output contracts](./cli/json-output.md) +* [Dev Instance files](./cli/dev-instance-agent-workflow.md) * [Performance](./cli/performance.md) * [Debugging](./cli/debugging.md) * [ESLint rules](./cli/eslint-rules.md) diff --git a/docs/cli/dev-instance-agent-workflow.md b/docs/cli/dev-instance-agent-workflow.md new file mode 100644 index 00000000000..295035e7738 --- /dev/null +++ b/docs/cli/dev-instance-agent-workflow.md @@ -0,0 +1,64 @@ +# Dev Instance files + +Long-running dev commands write an instance JSON file and a JSONL log under +`/.shopify/dev/`. Agents can read these files when dev was started in +another terminal. There is no IPC endpoint or extra command. + +The instance JSON includes the command, project directory, PID, `sessionId`, +`status`, `logFile`, and command-specific `details`. `status` is `starting` or +`running`. Use `logFile` to find the log; filenames include an identity hash. +A saved instance file alone does not prove that the process is alive. Check its +PID and lock heartbeat before reusing the session. + +## Read new output + +Each complete JSONL line contains `timestamp` and `message`, with `source` and +`level` where available. Terminal colors, spinners, and screen redraws are excluded. +Keep a byte offset to read new lines. Wait for an incomplete final line to finish. +If `sessionId` in the instance JSON changes, reset the offset: each new run clears +the log. Logs have no size limit or rotation. + +## Agent workflow + +1. Find the instance JSON for the intended command and configuration. Check its + identity details, PID, and lock heartbeat. +2. Reuse an existing session. If none is available, start the dev command through + the agent's background terminal facility. Supply the required configuration and + store flags and use `--no-input` when setup and authentication are available. +3. If a second start reports `dev_instance_already_running`, read the reported + instance and log. Do not stop a session started by the developer. +4. Make the change, read new output, inspect the command-specific status, and test + the result. Use a bounded wait and report errors or timeouts. + +Reported status is a snapshot of the CLI's state, not a health check or proof that +an edit has reached the store. Web-server and browser health require their own checks. + +## Shutdown and recovery + +Normal shutdown removes the instance JSON and lock, and retains the log until the +next run. Ctrl+C cancels dev work and waits for queued log writes. On Unix, +SIGINT and SIGTERM use the same shutdown path. Sending these signals through +`process.kill()` on Windows forces termination and does not drain queued writes. + +A live PID blocks takeover, including after the fixed 15-second stale interval. +Stop an unresponsive owner before starting another instance. Crashes after +startup recover automatically on the next start. If the CLI is killed during the +short lock-acquisition step, a `.lock.acquire` directory can remain. The next +start reports its path. Confirm that no other startup is running before removing +that directory and trying again. + +A crash can leave stale metadata or an incomplete log. Keep these local files out +of commits and external reports; output and local URLs can contain secrets. + +## App dev + +`shopify app dev` owns one session per app configuration. The instance details +include the app configuration, app identity, store, URLs, and `preview`: + +- `state`: `starting`, `updating`, `ready`, or `error`. +- `message`: the current CLI status message, or `null` before one is available. +- `errors`: the current reported errors. Errors remain during retries and clear + when the CLI reports success. + +Process messages keep their terminal source, including extension handles. +Changes to the status message also produce a log record with source `app dev`. diff --git a/package.json b/package.json index a16442d5847..ed92b5fd19c 100644 --- a/package.json +++ b/package.json @@ -117,7 +117,10 @@ "protobufjs", "unrs-resolver", "yarn" - ] + ], + "patchedDependencies": { + "when-exit@2.1.5": "patches/when-exit@2.1.5.patch" + } }, "resolutions": { "@types/react": "18.3.12", diff --git a/packages/app/src/cli/commands/app/dev.test.ts b/packages/app/src/cli/commands/app/dev.test.ts index 098e329f753..a20efa5e5bd 100644 --- a/packages/app/src/cli/commands/app/dev.test.ts +++ b/packages/app/src/cli/commands/app/dev.test.ts @@ -15,10 +15,18 @@ import { testProject, } from '../../models/app/app.test-data.js' import {inTemporaryDirectory} from '@shopify/cli-kit/node/fs' +import { + acquireDevInstance, + DEV_INSTANCE_ALREADY_RUNNING_CODE, + readDevInstance, +} from '@shopify/cli-kit/node/dev-instance' import {addPublicMetadata} from '@shopify/cli-kit/node/metadata' import {beforeEach, describe, expect, test, vi} from 'vitest' -vi.mock('../../services/dev.js') +vi.mock('../../services/dev.js', async (importOriginal) => ({ + ...(await importOriginal()), + dev: vi.fn(), +})) vi.mock('../../services/dev-clean.js') vi.mock('../../services/app-context.js') vi.mock('../../services/store-context.js') @@ -160,4 +168,39 @@ describe('app dev command', () => { expect(devClean).not.toHaveBeenCalled() }) }) + test('records the running instance while dev runs and releases it afterwards', async () => { + await inTemporaryDirectory(async (tmp) => { + const {store} = mockAppAndStore(tmp) + vi.mocked(getTunnelMode).mockResolvedValue({mode: 'auto'}) + const identity = {command: 'app dev', directory: tmp, key: 'shopify.app.toml'} + let instanceWhileRunning: Awaited> + vi.mocked(dev).mockImplementation(async () => { + instanceWhileRunning = await readDevInstance(identity) + }) + + await Dev.run(['--path', tmp, '--store', store.shopDomain], import.meta.url) + + expect(instanceWhileRunning).toMatchObject({command: 'app dev', key: 'shopify.app.toml', pid: process.pid}) + expect(dev).toHaveBeenCalledWith(expect.objectContaining({devInstance: expect.any(Object)})) + await expect(readDevInstance(identity)).resolves.toBeUndefined() + }) + }) + + test('refuses to start when app dev is already running for the same configuration', async () => { + await inTemporaryDirectory(async (tmp) => { + const {store} = mockAppAndStore(tmp) + vi.mocked(getTunnelMode).mockResolvedValue({mode: 'auto'}) + const running = await acquireDevInstance({command: 'app dev', directory: tmp, key: 'shopify.app.toml'}) + + // The command's error handler renders the AbortError and exits. + await expect(Dev.run(['--path', tmp, '--store', store.shopDomain], import.meta.url)).rejects.toThrow() + await expect( + acquireDevInstance({command: 'app dev', directory: tmp, key: 'shopify.app.toml'}), + ).rejects.toMatchObject({code: DEV_INSTANCE_ALREADY_RUNNING_CODE, details: {instance: running.info}}) + + expect(storeContext).not.toHaveBeenCalled() + expect(dev).not.toHaveBeenCalled() + await running.release() + }) + }) }) diff --git a/packages/app/src/cli/commands/app/dev.ts b/packages/app/src/cli/commands/app/dev.ts index 6fe12af0385..a32290c0cff 100644 --- a/packages/app/src/cli/commands/app/dev.ts +++ b/packages/app/src/cli/commands/app/dev.ts @@ -1,5 +1,5 @@ import {appFlags} from '../../flags.js' -import {dev, DevOptions} from '../../services/dev.js' +import {dev, DevOptions, withDevSession} from '../../services/dev.js' import {checkFolderIsValidApp} from '../../models/app/loader.js' import AppLinkedCommand, {AppLinkedCommandOutput} from '../../utilities/app-linked-command.js' import {linkedAppContext} from '../../services/app-context.js' @@ -142,35 +142,39 @@ export default class Dev extends AppLinkedCommand { forceRelink: flags.reset, userProvidedConfigName: flags.config, }) - const store = await storeContext({ - appContextResult, - storeFqdn: flags.store, - forceReselectStore: flags.reset, - storeCreationMode: 'selection-option', - }) - const devOptions: DevOptions = { - ...appContextResult, - store, - directory: flags.path, - update: !flags['no-update'], - unsafeValidation: flags['unsafe-validation'], - skipDependenciesInstallation: flags['skip-dependencies-installation'], - commandConfig: this.config, - subscriptionProductUrl: flags['subscription-product-url'], - checkoutCartUrl: flags['checkout-cart-url'], - theme: flags.theme, - themeExtensionPort: flags['theme-app-extension-port'], - storePassword: flags['store-password'], - notify: flags.notify, - graphiqlPort: flags['graphiql-port'], - graphiqlKey: flags['graphiql-key'], - // Must stay undefined when the flag is absent: generateCertificate() only prompts when this is nullish. - installMkcert: flags['install-mkcert'], - tunnel: tunnelMode, - } + return withDevSession(appContextResult, async (devInstance) => { + const store = await storeContext({ + appContextResult, + storeFqdn: flags.store, + forceReselectStore: flags.reset, + storeCreationMode: 'selection-option', + }) - await dev(devOptions) - return {app: appContextResult.app} + const devOptions: DevOptions = { + ...appContextResult, + store, + directory: flags.path, + update: !flags['no-update'], + unsafeValidation: flags['unsafe-validation'], + skipDependenciesInstallation: flags['skip-dependencies-installation'], + commandConfig: this.config, + subscriptionProductUrl: flags['subscription-product-url'], + checkoutCartUrl: flags['checkout-cart-url'], + theme: flags.theme, + themeExtensionPort: flags['theme-app-extension-port'], + storePassword: flags['store-password'], + notify: flags.notify, + graphiqlPort: flags['graphiql-port'], + graphiqlKey: flags['graphiql-key'], + // Must stay undefined when the flag is absent: generateCertificate() only prompts when this is nullish. + installMkcert: flags['install-mkcert'], + tunnel: tunnelMode, + devInstance, + } + + await dev(devOptions) + return {app: appContextResult.app} + }) } } diff --git a/packages/app/src/cli/services/dev.test.ts b/packages/app/src/cli/services/dev.test.ts index 119576f884d..857a528c272 100644 --- a/packages/app/src/cli/services/dev.test.ts +++ b/packages/app/src/cli/services/dev.test.ts @@ -2,6 +2,8 @@ import {dev, blockIfMigrationIncomplete} from './dev.js' import {setupDevProcesses} from './dev/processes/setup-dev-processes.js' import {renderDev} from './dev/ui.js' import {fetchAppRemoteConfiguration} from './app/select-app.js' +import {DevSessionStatusManager} from './dev/processes/dev-session/dev-session-status-manager.js' +import {DevSessionLogger} from './dev/processes/dev-session/dev-session-logger.js' import { testAppLinked, testDeveloperPlatformClient, @@ -15,7 +17,12 @@ import {describe, expect, test, vi} from 'vitest' import {hashString} from '@shopify/cli-kit/node/crypto' import {reportAnalyticsEvent} from '@shopify/cli-kit/node/analytics' import {checkPortAvailability, getAvailableTCPPort} from '@shopify/cli-kit/node/tcp' +import {acquireDevInstance} from '@shopify/cli-kit/node/dev-instance' +import {inTemporaryDirectory, readFile} from '@shopify/cli-kit/node/fs' +import {useConcurrentOutputContext} from '@shopify/cli-kit/node/ui/components' +import {Writable} from 'stream' +vi.mock('./local-storage.js') vi.mock('./dev/fetch.js') vi.mock('./dev/processes/setup-dev-processes.js') vi.mock('./dev/ui.js') @@ -86,6 +93,129 @@ describe('dev', () => { }) }) +describe('dev with a dev instance', () => { + test.each([false, true])('captures output and session URLs, stopping on lock loss: %s', async (loseLock) => { + await inTemporaryDirectory(async (tmp) => { + const store = testOrganizationStore({shopDomain: 'dev-store.myshopify.com'}) + const app = testAppLinked({directory: tmp}) + const manager = new DevSessionStatusManager({ + isReady: false, + statusMessage: {type: 'loading', message: 'Preparing dev preview'}, + }) + const devInstance = await acquireDevInstance({command: 'app dev', directory: tmp}) + const ownership = new AbortController() + const previewUrl = 'https://dev-store.myshopify.com/admin/apps/api-key' + const graphiqlUrl = 'http://localhost:3456/graphiql' + let infoWhileRunning: unknown + + vi.mocked(fetchAppRemoteConfiguration).mockResolvedValue({ + name: 'Remote app', + application_url: '', + embedded: true, + }) + vi.mocked(getAvailableTCPPort).mockResolvedValue(3456) + vi.mocked(checkPortAvailability).mockResolvedValue(true) + vi.mocked(setupDevProcesses).mockResolvedValue({ + processes: [ + { + type: 'web', + prefix: 'web', + function: async ({stdout, stderr}: {stdout: Writable; stderr: Writable}) => { + stdout.write('Server listening on 3000\n') + stderr.write('Deprecation warning\n') + useConcurrentOutputContext({outputPrefix: 'discount'}, () => { + stdout.write('Building function discount...') + stdout.write('Building GraphQL types...\n') + }) + await new DevSessionLogger(stdout).info('Update accepted') + }, + options: {}, + } as any, + ], + previewUrl, + graphiqlUrl, + devSessionStatusManager: manager, + }) + const terminal = new Writable({write: (_chunk, _encoding, callback) => callback()}) + vi.mocked(renderDev).mockImplementation(async ({processes, abortController}) => { + manager.updateStatus({previewURL: previewUrl}) + manager.setMessage('READY') + manager.setMessage('CHANGE_DETECTED') + manager.updateStatus({isReady: true}) + manager.setMessage('BUILD_ERROR', ['discount: Invalid input']) + manager.updateStatus({graphiqlURL: graphiqlUrl}) + await vi.waitFor(async () => { + infoWhileRunning = JSON.parse(await readFile(devInstance.paths.infoFile)) + expect(infoWhileRunning).toHaveProperty('details.preview.errors', ['discount: Invalid input']) + }) + await Promise.all(processes.map((process) => process.action(terminal, terminal, new AbortController().signal))) + if (loseLock) { + ownership.abort(new Error('Lock lost')) + expect(abortController.signal.aborted).toBe(true) + } + }) + + const running = dev({ + app, + project: testProject({usesWorkspaces: true}), + remoteApp: testOrganizationApp({apiKey: 'api-key'}), + organization: testOrganization(), + specifications: [], + developerPlatformClient: testDeveloperPlatformClient(), + store, + directory: app.directory, + update: false, + commandConfig: {} as any, + skipDependenciesInstallation: true, + tunnel: {mode: 'custom', url: 'https://localhost:3456'}, + devInstance: {...devInstance, abortSignal: ownership.signal}, + }) + if (loseLock) { + await expect(running).rejects.toThrow('Lock lost') + } else { + await running + } + await devInstance.release() + + expect(infoWhileRunning).toMatchObject({ + status: 'running', + details: { + storeFqdn: 'dev-store.myshopify.com', + previewUrl, + preview: {state: 'error', errors: ['discount: Invalid input']}, + }, + }) + const entries = (await readFile(devInstance.paths.logFile)) + .split('\n') + .filter(Boolean) + .map((line) => JSON.parse(line)) + entries.forEach((entry) => expect(entry).not.toHaveProperty('stream')) + expect(entries).toEqual( + expect.arrayContaining([ + expect.objectContaining({source: 'app dev', message: `Preview URL: ${previewUrl}`}), + expect.objectContaining({source: 'app dev', message: `GraphiQL URL (Admin API): ${graphiqlUrl}`}), + expect.objectContaining({source: 'web', message: 'Server listening on 3000'}), + expect.objectContaining({source: 'web', message: 'Deprecation warning'}), + expect.objectContaining({source: 'discount', message: 'Building function discount...'}), + expect.objectContaining({source: 'discount', message: 'Building GraphQL types...'}), + expect.objectContaining({source: 'app-preview', message: 'Update accepted'}), + ]), + ) + expect( + entries + .filter(({source}) => source === 'app dev') + .filter(({message}) => !message.startsWith('Preview URL:') && !message.startsWith('GraphiQL URL')) + .map(({level, message}) => ({level, message})), + ).toEqual([ + {level: 'info', message: 'Preparing dev preview'}, + {level: 'info', message: 'Ready, watching for changes in your app'}, + {level: 'info', message: 'Change detected, updating dev preview'}, + {level: 'error', message: 'Build error. Please review your code and try again'}, + ]) + }) + }) +}) + describe('blockIfMigrationIncomplete', () => { const baseConfig = () => ({ localApp: testAppLinked({}), diff --git a/packages/app/src/cli/services/dev.ts b/packages/app/src/cli/services/dev.ts index e5a81c4ad65..ef292d7dc30 100644 --- a/packages/app/src/cli/services/dev.ts +++ b/packages/app/src/cli/services/dev.ts @@ -15,7 +15,7 @@ import {renderDev} from './dev/ui.js' import {DevProcessFunction} from './dev/processes/types.js' import {getCachedAppInfo, setCachedAppInfo} from './local-storage.js' import {fetchAppRemoteConfiguration} from './app/select-app.js' -import {DevSessionStatusManager} from './dev/processes/dev-session/dev-session-status-manager.js' +import {DevSessionStatus, DevSessionStatusManager} from './dev/processes/dev-session/dev-session-status-manager.js' import {TunnelMode} from './dev/tunnel-mode.js' import {PortDetail, renderPortWarnings} from './dev/port-warnings.js' import {DeveloperPlatformClient} from '../utilities/developer-platform-client.js' @@ -36,9 +36,11 @@ import {checkPortAvailability, getAvailableTCPPort} from '@shopify/cli-kit/node/ import {TunnelClient} from '@shopify/cli-kit/node/plugins/tunnel' import {getBackendPort} from '@shopify/cli-kit/node/environment' import {reportAnalyticsEvent} from '@shopify/cli-kit/node/analytics' -import {OutputProcess} from '@shopify/cli-kit/node/output' +import {OutputProcess, outputDebug} from '@shopify/cli-kit/node/output' import {hashString} from '@shopify/cli-kit/node/crypto' import {AbortError} from '@shopify/cli-kit/node/error' +import {withDevInstance, type DevInstance} from '@shopify/cli-kit/node/dev-instance' +import {basename} from '@shopify/cli-kit/node/path' export interface DevOptions { app: AppLinkedInterface @@ -63,6 +65,24 @@ export interface DevOptions { graphiqlPort?: number graphiqlKey?: string installMkcert?: boolean + /** The running-instance record for this session: its log receives every process's output. */ + devInstance?: DevInstance +} + +/** Runs app development while the configuration's dev instance is owned. */ +export function withDevSession( + {app, remoteApp}: Pick, + execute: (instance: DevInstance) => T | Promise, +): Promise { + return withDevInstance( + { + command: 'app dev', + directory: app.directory, + key: basename(app.configPath), + details: {configPath: app.configPath, appName: remoteApp.title, clientId: remoteApp.apiKey}, + }, + execute, + ) } export async function dev(commandOptions: DevOptions) { @@ -328,37 +348,100 @@ async function launchDevProcesses({ devSessionStatusManager: DevSessionStatusManager }) { const abortController = new AbortController() + const devInstance = config.commandOptions.devInstance const processesForTaskRunner: OutputProcess[] = processes.map((process) => { const outputProcess: OutputProcess = { prefix: process.prefix, action: async (stdout, stderr, signal) => { const fn = process.function as DevProcessFunction - return fn({stdout, stderr, abortSignal: signal}, process.options) + const processStdout = devInstance + ? devInstance.tee(stdout, {source: process.prefix, stringWritesAreMessages: true}) + : stdout + const processStderr = devInstance + ? devInstance.tee(stderr, {source: process.prefix, stringWritesAreMessages: true}) + : stderr + return fn({stdout: processStdout, stderr: processStderr, abortSignal: signal}, process.options) }, } return outputProcess }) + if (devInstance) { + const urls = { + storeFqdn: config.storeFqdn, + previewUrl, + graphiqlUrl, + appUrl: config.localApp.devApplicationURLs?.applicationUrl, + localUrl: config.network.proxyUrl, + } + await devInstance.update({ + status: 'running', + details: {...urls, preview: previewStatus(devSessionStatusManager.status)}, + }) + await devInstance.log({source: 'app dev', level: 'info', message: `Preview URL: ${previewUrl}`}) + if (graphiqlUrl) { + await devInstance.log({source: 'app dev', level: 'info', message: `GraphiQL URL (Admin API): ${graphiqlUrl}`}) + } + } + const developerPlatformClient = config.developerPlatformClient const app = { id: config.remoteApp.id, developerPlatformClient, } - return renderDev({ - processes: processesForTaskRunner, - previewUrl, - graphiqlUrl, - app, - abortController, - shopFqdn: config.storeFqdn, - devSessionStatusManager, - appURL: config.localApp.devApplicationURLs?.applicationUrl, - appName: config.remoteApp.title, - organizationName: config.commandOptions.organization.businessName, - configPath: config.localApp.configPath, - localURL: config.network.proxyUrl, - }) + let lastStatusMessage: DevSessionStatus['statusMessage'] + const persistStatus = (status: DevSessionStatus) => { + const writes = [devInstance?.update({details: {preview: previewStatus(status)}})] + const message = status.statusMessage + if (message && (message.message !== lastStatusMessage?.message || message.type !== lastStatusMessage?.type)) { + writes.push( + devInstance?.log({ + source: 'app dev', + level: message.type === 'error' ? 'error' : 'info', + message: message.message, + }), + ) + } + lastStatusMessage = message + Promise.all(writes).catch((error: Error) => { + outputDebug(`Failed to save dev status: ${error.message}`) + }) + } + if (devInstance) { + devSessionStatusManager.on('dev-session-update', persistStatus) + persistStatus(devSessionStatusManager.status) + } + const stopOnLostLock = () => abortController.abort(devInstance?.abortSignal.reason) + try { + devInstance?.abortSignal.throwIfAborted() + devInstance?.abortSignal.addEventListener('abort', stopOnLostLock, {once: true}) + await renderDev({ + processes: processesForTaskRunner, + previewUrl, + graphiqlUrl, + app, + abortController, + shopFqdn: config.storeFqdn, + devSessionStatusManager, + appURL: config.localApp.devApplicationURLs?.applicationUrl, + appName: config.remoteApp.title, + organizationName: config.commandOptions.organization.businessName, + configPath: config.localApp.configPath, + localURL: config.network.proxyUrl, + }) + devInstance?.abortSignal.throwIfAborted() + } finally { + devInstance?.abortSignal.removeEventListener('abort', stopOnLostLock) + if (devInstance) devSessionStatusManager.off('dev-session-update', persistStatus) + } +} + +function previewStatus({isReady, statusMessage, errors}: DevSessionStatus) { + let state = isReady ? 'ready' : 'starting' + if (isReady && statusMessage?.type === 'loading') state = 'updating' + if (statusMessage?.type === 'error') state = 'error' + return {state, message: statusMessage?.message ?? null, errors: errors ?? []} } async function logMetadataForDev(options: { diff --git a/packages/app/src/cli/services/dev/processes/dev-session/dev-session-logger.test.ts b/packages/app/src/cli/services/dev/processes/dev-session/dev-session-logger.test.ts index f00878f1a63..66dce20eb7e 100644 --- a/packages/app/src/cli/services/dev/processes/dev-session/dev-session-logger.test.ts +++ b/packages/app/src/cli/services/dev/processes/dev-session/dev-session-logger.test.ts @@ -44,7 +44,8 @@ describe('DevSessionLogger', () => { output = [] stdout = { write: (message: string) => { - output.push(message) + expect(message.endsWith('\n')).toBe(true) + output.push(message.slice(0, -1)) return true }, } as unknown as Writable diff --git a/packages/app/src/cli/services/dev/processes/dev-session/dev-session-logger.ts b/packages/app/src/cli/services/dev/processes/dev-session/dev-session-logger.ts index 93bed1786ff..b997deb39c6 100644 --- a/packages/app/src/cli/services/dev/processes/dev-session/dev-session-logger.ts +++ b/packages/app/src/cli/services/dev/processes/dev-session/dev-session-logger.ts @@ -112,7 +112,7 @@ export class DevSessionLogger { // Helper function to print to terminal using output context with stripAnsi disabled. private async log(message: string, prefix?: string) { await useConcurrentOutputContext({outputPrefix: prefix ?? 'app-preview', stripAnsi: false}, () => { - this.stdout.write(message) + this.stdout.write(`${message}\n`) }) } } diff --git a/packages/app/src/cli/services/dev/processes/dev-session/dev-session-status-manager.test.ts b/packages/app/src/cli/services/dev/processes/dev-session/dev-session-status-manager.test.ts index 0b577f0bcaf..532dc433296 100644 --- a/packages/app/src/cli/services/dev/processes/dev-session/dev-session-status-manager.test.ts +++ b/packages/app/src/cli/services/dev/processes/dev-session/dev-session-status-manager.test.ts @@ -140,3 +140,12 @@ describe('DevSessionStatusManager', () => { }) }) }) + +test('keeps current errors during an update and clears them after success', () => { + const manager = new DevSessionStatusManager() + manager.setMessage('BUILD_ERROR', ['discount: Invalid input']) + manager.setMessage('CHANGE_DETECTED') + expect(manager.status.errors).toEqual(['discount: Invalid input']) + manager.setMessage('UPDATED') + expect(manager.status.errors).toEqual([]) +}) diff --git a/packages/app/src/cli/services/dev/processes/dev-session/dev-session-status-manager.ts b/packages/app/src/cli/services/dev/processes/dev-session/dev-session-status-manager.ts index 7d41b0d3f7d..23ac28baec6 100644 --- a/packages/app/src/cli/services/dev/processes/dev-session/dev-session-status-manager.ts +++ b/packages/app/src/cli/services/dev/processes/dev-session/dev-session-status-manager.ts @@ -15,6 +15,7 @@ const DevSessionStaticMessages = { export interface DevSessionStatus { isReady: boolean + errors?: string[] previewURL?: string graphiqlURL?: string appEmbedded?: boolean @@ -44,8 +45,10 @@ export class DevSessionStatusManager extends EventEmitter { this.emit('dev-session-update', newStatus) } - setMessage(message: keyof typeof DevSessionStaticMessages) { - this.updateStatus({statusMessage: DevSessionStaticMessages[message]}) + setMessage(message: keyof typeof DevSessionStaticMessages, errors?: string[]) { + const statusMessage = DevSessionStaticMessages[message] + const nextErrors = statusMessage.type === 'success' ? [] : errors + this.updateStatus({statusMessage, ...(nextErrors ? {errors: nextErrors} : {})}) } get status(): DevSessionStatus { diff --git a/packages/app/src/cli/services/dev/processes/dev-session/dev-session.ts b/packages/app/src/cli/services/dev/processes/dev-session/dev-session.ts index d799ddf127f..0a9dbc318f7 100644 --- a/packages/app/src/cli/services/dev/processes/dev-session/dev-session.ts +++ b/packages/app/src/cli/services/dev/processes/dev-session/dev-session.ts @@ -162,6 +162,10 @@ export class DevSession { private async onStart(event: AppEvent) { const errors = this.parseBuildErrors(event) if (errors.length) { + this.statusManager.setMessage( + 'BUILD_ERROR', + errors.map(({prefix, error}) => `${prefix}: ${error}`), + ) await this.logger.logMultipleErrors(errors) await setImmediate(() => { const affected = errors.map((err) => err.prefix) @@ -193,7 +197,10 @@ export class DevSession { const errors = this.parseBuildErrors(event) if (errors.length) { await this.logger.logMultipleErrors(errors) - this.statusManager.setMessage('BUILD_ERROR') + this.statusManager.setMessage( + 'BUILD_ERROR', + errors.map(({prefix, error}) => `${prefix}: ${error}`), + ) return false } @@ -215,7 +222,10 @@ export class DevSession { private parseBuildErrors(event: AppEvent) { const buildErrors = event.extensionEvents.filter((eve) => eve.buildResult?.status === 'error') return buildErrors.map((error) => ({ - error: 'Build error. Please review your code and try again.', + error: + error.buildResult?.status === 'error' + ? error.buildResult.error || 'Build error. Please review your code and try again.' + : 'Build error. Please review your code and try again.', prefix: error.extension.handle, })) } @@ -237,12 +247,13 @@ export class DevSession { await this.logger.logExtensionUpdateMessages(event, result) this.statusManager.setMessage('READY') } else if (result.status === 'remote-error' || result.status === 'unknown-error') { + const errors = result.error instanceof Error ? [result.error.message] : result.error.map((error) => error.message) await this.logger.logUserErrors(result.error, event?.app.allExtensions ?? []) if (result.error instanceof Error && (result.error as Error & {cause?: string}).cause === 'validation-error') { - this.statusManager.setMessage('VALIDATION_ERROR') + this.statusManager.setMessage('VALIDATION_ERROR', errors) } else { if (event) this.failedEvents.push(event) - this.statusManager.setMessage('REMOTE_ERROR') + this.statusManager.setMessage('REMOTE_ERROR', errors) } } diff --git a/packages/app/src/cli/services/dev/ui.integration.test.ts b/packages/app/src/cli/services/dev/ui.integration.test.ts new file mode 100644 index 00000000000..8879ed3ffd7 --- /dev/null +++ b/packages/app/src/cli/services/dev/ui.integration.test.ts @@ -0,0 +1,127 @@ +import {test, expect, vi} from 'vitest' +import {captureOutputWithExitCode} from '@shopify/cli-kit/node/system' +import {fileExists, inTemporaryDirectory, readFile} from '@shopify/cli-kit/node/fs' +import {devInstancePaths, isProcessAlive} from '@shopify/cli-kit/node/dev-instance' +import {fileURLToPath} from 'url' + +const bootstrapUrl = new URL('../../../../../cli/src/bootstrap.ts', import.meta.url).href +const serviceUrl = new URL('../dev.ts', import.meta.url).href +const uiUrl = new URL('./ui.tsx', import.meta.url).href +const statusUrl = new URL('./processes/dev-session/dev-session-status-manager.ts', import.meta.url).href +const postrunUrl = new URL('../../../../../cli-kit/src/public/node/hooks/postrun.ts', import.meta.url).href +const sourceLoaderUrl = new URL('../../../../test/fixtures/app-source-loader.js', import.meta.url).href +const tsconfigPath = fileURLToPath(new URL('../../../../test/fixtures/tsconfig.json', import.meta.url)) +// Windows starts PowerShell to enumerate and stop the remaining child processes. +const postrunExitTimeoutMs = process.platform === 'win32' ? 15000 : 5000 + +// Windows process.kill() terminates immediately instead of delivering a catchable signal. +// Exercise handlers on every platform and OS signal delivery where it is supported. +const shutdownCases = ['SIGINT', 'SIGTERM'].flatMap((signal) => + (process.platform === 'win32' ? ['emit'] : ['emit', 'kill']).map((delivery) => ({signal, delivery})), +) + +test.each(shutdownCases)( + '$delivery $signal keeps the default bootstrap exit when no dev session is running', + {timeout: 20000}, + async ({signal, delivery}) => { + const script = ` + await import(${JSON.stringify(bootstrapUrl)}) + setInterval(() => {}, 1000) + process.${delivery}(${delivery === 'kill' ? 'process.pid, ' : ''}${JSON.stringify(signal)}) + ` + const result = await captureOutputWithExitCode( + process.execPath, + ['--loader', 'ts-node/esm', '--loader', sourceLoaderUrl, '--input-type=module', '--eval', script], + {env: {...process.env, NODE_NO_WARNINGS: '1', SHOPIFY_CLI_ENV: 'development', TS_NODE_PROJECT: tsconfigPath}}, + ) + expect(result.exitCode, result.stderr).toBe(1) + expect(result.stderr).toBe('') + }, +) + +test.each(shutdownCases)( + '$delivery $signal flushes queued output and exits with background work after the postrun hook', + // Source loading has its own allowance; the child separately bounds shutdown after the signal. + {timeout: 60000}, + async ({signal, delivery}) => { + await inTemporaryDirectory(async (directory) => { + const script = ` + await import(${JSON.stringify(bootstrapUrl)}) + const {withDevSession} = await import(${JSON.stringify(serviceUrl)}) + const {renderDev} = await import(${JSON.stringify(uiUrl)}) + const {DevSessionStatusManager} = await import(${JSON.stringify(statusUrl)}) + const {hook} = await import(${JSON.stringify(postrunUrl)}) + const {spawn} = await import('node:child_process') + await withDevSession({ + app: {directory: ${JSON.stringify(directory)}, configPath: 'shopify.app.toml'}, + remoteApp: {title: 'Local fixture', apiKey: 'fixture'}, + }, async (instance) => { + const abortController = new AbortController() + await renderDev({ + processes: [{prefix: 'web', action: async (stdout, _stderr, signal) => { + setInterval(() => {}, 1000) + const child = spawn(process.execPath, ['--eval', 'setInterval(() => {}, 1000); setTimeout(() => process.exit(2), 30000)'], {stdio: 'ignore'}) + process.stderr.write('Background PID: ' + child.pid + '\\n') + const output = instance.tee(stdout, {source: 'web'}) + for (let index = 0; index < 1000; index++) output.write('line ' + index + '\\n') + const stopped = new Promise((resolve) => signal.addEventListener('abort', () => { + output.write('Stopped\\n') + resolve() + }, {once: true})) + setTimeout(() => { + process.stderr.write('Dev shutdown timed out\\n') + process.exit(2) + }, 20000) + process.${delivery}(${delivery === 'kill' ? 'process.pid, ' : ''}${JSON.stringify(signal)}) + await stopped + }}], + previewUrl: 'https://example.com', + app: {id: 'fixture', developerPlatformClient: {}}, + abortController, + shopFqdn: 'fixture.myshopify.com', + devSessionStatusManager: new DevSessionStatusManager(), + }) + }) + process.stdout.write('Session cleaned up\\n') + await hook.call({}, {config: {}, Command: {id: 'app:dev'}}) + process.stdout.write('Postrun completed\\n') + setTimeout(() => { + process.stderr.write('Postrun shutdown timed out\\n') + process.exit(2) + }, ${postrunExitTimeoutMs}) + ` + const result = await captureOutputWithExitCode( + process.execPath, + ['--loader', 'ts-node/esm', '--loader', sourceLoaderUrl, '--input-type=module', '--eval', script], + { + env: { + ...process.env, + NODE_NO_WARNINGS: '1', + SHOPIFY_CLI_ENV: 'development', + SHOPIFY_CLI_NO_ANALYTICS: '1', + SHOPIFY_CLI_FORCE_AUTO_UPGRADE: '0', + CI: '1', + TS_NODE_PROJECT: tsconfigPath, + }, + }, + ) + const childPid = Number(result.stderr.match(/Background PID: (\d+)/)?.[1]) + expect(childPid).toBeGreaterThan(0) + await vi.waitFor(() => expect(isProcessAlive(childPid)).toBe(false)) + expect(result.exitCode, result.stderr).toBe(0) + expect(result.stdout).toContain('Session cleaned up') + expect(result.stdout).toContain('Postrun completed') + const paths = devInstancePaths({command: 'app dev', directory, key: 'shopify.app.toml'}) + 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) => `line ${index}`), + 'Stopped', + ]) + await expect(fileExists(paths.infoFile)).resolves.toBe(false) + await expect(fileExists(paths.lockFile)).resolves.toBe(false) + }) + }, +) diff --git a/packages/app/src/cli/services/dev/ui.test.tsx b/packages/app/src/cli/services/dev/ui.test.tsx index ad6e27c29e8..0d35081b638 100644 --- a/packages/app/src/cli/services/dev/ui.test.tsx +++ b/packages/app/src/cli/services/dev/ui.test.tsx @@ -3,11 +3,14 @@ import {DevSessionUI} from './ui/components/DevSessionUI.js' import {DevSessionStatusManager} from './processes/dev-session/dev-session-status-manager.js' import {testDeveloperPlatformClient} from '../../models/app/app.test-data.js' import {afterEach, describe, expect, test, vi} from 'vitest' -import {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output' +import {mockAndCaptureOutput, withCapturedStandardStreams} from '@shopify/cli-kit/node/testing/output' import {AbortController} from '@shopify/cli-kit/node/abort' import {terminalSupportsPrompting} from '@shopify/cli-kit/node/system' +import {outputWarn, unstyled} from '@shopify/cli-kit/node/output' +import {Writable} from 'stream' vi.mock('@shopify/cli-kit/node/system') +vi.mock('@shopify/cli-kit/node/hooks/postrun', () => ({waitForPostRunHookAndExit: vi.fn()})) vi.mock('./ui/components/DevSessionUI.js') const developerPlatformClient = testDeveloperPlatformClient() @@ -19,6 +22,84 @@ afterEach(() => { describe('ui', () => { describe('renderDev', () => { + test('separates CLI messages while preserving line endings and subprocess bytes', async () => { + vi.mocked(terminalSupportsPrompting).mockReturnValue(false) + const abortController = new AbortController() + await withCapturedStandardStreams(async ({stdout, stderr}) => { + await renderDev({ + processes: [ + { + prefix: 'app-logs', + action: async (output, errorOutput) => { + await Promise.all( + [output, errorOutput].map(async (stream) => { + stream.write('') + outputWarn('Error while polling app logs.', stream) + outputWarn('Retrying in 5 seconds.', stream) + stream.write('Existing line ending\r\n') + stream.write('636166c3a9', 'hex') + const unicode = Buffer.from('éclair\n') + stream.write(unicode.subarray(0, 1)) + await new Promise((resolve, reject) => + stream.write(unicode.subarray(1), (error) => (error ? reject(error) : resolve())), + ) + }), + ) + abortController.abort() + }, + }, + ], + previewUrl: 'https://example.com', + app: {id: '123', developerPlatformClient}, + abortController, + shopFqdn: 'dev-store.myshopify.com', + devSessionStatusManager: new DevSessionStatusManager(), + }) + const expected = 'Error while polling app logs.\nRetrying in 5 seconds.\nExisting line ending\r\ncafé\néclair\n' + expect(unstyled(stdout())).toBe(`\nPreview URL: https://example.com\n${expected}`) + expect(unstyled(stderr())).toBe(expected) + }) + }) + + test('a process failure aborts and waits for the other processes to stop', async () => { + vi.mocked(terminalSupportsPrompting).mockReturnValue(false) + const abortController = new AbortController() + const failure = new Error('Process failed') + let stopped = false + const signalListeners = process.listenerCount('SIGTERM') + const shutdownListeners = process.listenerCount('shopify:signal') + await withCapturedStandardStreams(async () => { + await expect( + renderDev({ + processes: [ + { + prefix: 'failed', + action: async () => { + throw failure + }, + }, + { + prefix: 'watcher', + action: async (_stdout, _stderr, signal) => { + await new Promise((resolve) => signal.addEventListener('abort', () => resolve(), {once: true})) + await new Promise((resolve) => setTimeout(resolve, 5)) + stopped = true + }, + }, + ], + previewUrl: 'https://example.com', + app: {id: '123', developerPlatformClient}, + abortController, + shopFqdn: 'dev-store.myshopify.com', + devSessionStatusManager: new DevSessionStatusManager(), + }), + ).rejects.toBe(failure) + }) + expect(stopped).toBe(true) + expect(process.listenerCount('SIGTERM')).toBe(signalListeners) + expect(process.listenerCount('shopify:signal')).toBe(shutdownListeners) + }) + test("doesn't use ink when terminal doesn't support TTY", async () => { vi.mocked(terminalSupportsPrompting).mockReturnValue(false) const concurrentProcess = { @@ -27,7 +108,8 @@ describe('ui', () => { } const abortController = new AbortController() - await renderDev({ + const completed = vi.fn() + const running = renderDev({ processes: [concurrentProcess], previewUrl: 'https://lala.cloudflare.io/', graphiqlUrl: 'https://lala.cloudflare.io/graphiql', @@ -35,12 +117,17 @@ describe('ui', () => { abortController, shopFqdn: 'mystore.shopify.io', devSessionStatusManager, - }) + }).then(completed) + await Promise.resolve() + await Promise.resolve() + expect(completed).not.toHaveBeenCalled() + abortController.abort() + await running expect(concurrentProcess.action).toHaveBeenNthCalledWith( 1, - process.stdout, - process.stderr, + expect.any(Writable), + expect.any(Writable), abortController.signal, ) }) @@ -48,9 +135,12 @@ describe('ui', () => { test("shows preview and GraphiQL URLs when terminal doesn't support TTY", async () => { vi.mocked(terminalSupportsPrompting).mockReturnValue(false) const write = vi.spyOn(process.stdout, 'write').mockImplementation(() => true) + const abortController = new AbortController() const concurrentProcess = { prefix: 'prefix', - action: vi.fn(async (_stdout, _stderr, _signal) => {}), + action: vi.fn(async () => { + abortController.abort() + }), } await renderDev({ @@ -58,7 +148,7 @@ describe('ui', () => { previewUrl: 'https://lala.cloudflare.io/', graphiqlUrl: 'https://lala.cloudflare.io/graphiql', app: {id: '123', developerPlatformClient}, - abortController: new AbortController(), + abortController, shopFqdn: 'mystore.shopify.io', devSessionStatusManager, }) diff --git a/packages/app/src/cli/services/dev/ui.tsx b/packages/app/src/cli/services/dev/ui.tsx index 9eb23042433..fb6c709ff24 100644 --- a/packages/app/src/cli/services/dev/ui.tsx +++ b/packages/app/src/cli/services/dev/ui.tsx @@ -6,6 +6,8 @@ import {AbortController} from '@shopify/cli-kit/node/abort' import React from 'react' import {render} from '@shopify/cli-kit/node/ui' import {terminalSupportsPrompting} from '@shopify/cli-kit/node/system' +import {waitForPostRunHookAndExit} from '@shopify/cli-kit/node/hooks/postrun' +import {Writable} from 'stream' interface DevProps { processes: OutputProcess[] @@ -40,34 +42,41 @@ export async function renderDev({ configPath?: string localURL?: string }) { - if (terminalSupportsPrompting()) { - return render( - { - await app.developerPlatformClient.devSessionDelete({appId: app.id, shopFqdn}) - }} - />, - { - exitOnCtrlC: false, - }, - ) - } + const stop = () => abortController.abort() + process.on('shopify:signal', stop) + try { + if (terminalSupportsPrompting()) { + return await render( + { + await app.developerPlatformClient.devSessionDelete({appId: app.id, shopFqdn}) + }} + />, + { + exitOnCtrlC: false, + }, + ) + } - await renderDevNonInteractive({ - processes, - previewUrl, - graphiqlUrl, - abortController, - }) + await renderDevNonInteractive({ + processes, + previewUrl, + graphiqlUrl, + abortController, + }) + waitForPostRunHookAndExit() + } finally { + process.removeListener('shopify:signal', stop) + } } async function renderDevNonInteractive({ @@ -81,9 +90,41 @@ async function renderDevNonInteractive({ process.stdout.write(`GraphiQL URL (Admin API): ${graphiqlUrl}\n`) } - return Promise.all( - processes.map(async (concurrentProcess) => { - await concurrentProcess.action(process.stdout, process.stderr, abortController.signal) - }), + const {signal} = abortController + const stop = () => abortController.abort() + process.once('SIGINT', stop) + process.once('SIGTERM', stop) + let onAbort: () => void = () => {} + const stopped = new Promise((resolve) => { + onAbort = resolve + signal.addEventListener('abort', onAbort, {once: true}) + if (signal.aborted) resolve() + }) + const running = processes.map(async (concurrentProcess) => + concurrentProcess.action(createTextOutputStream(process.stdout), createTextOutputStream(process.stderr), signal), ) + try { + // A watcher can finish setup while it continues serving. Keep the session registered until abort. + await Promise.all([...running, stopped]) + } catch (error) { + if (!signal.aborted) throw error + } finally { + abortController.abort() + await Promise.allSettled(running) + signal.removeEventListener('abort', onAbort) + process.removeListener('SIGINT', stop) + process.removeListener('SIGTERM', stop) + } +} + +function createTextOutputStream(target: Writable): Writable { + return new Writable({ + decodeStrings: false, + write(chunk: string | Buffer, encoding, callback) { + // CLI string writes are complete messages; subprocess byte chunks can contain partial lines. + const output = typeof chunk === 'string' ? Buffer.from(chunk, encoding) : chunk + const needsNewline = typeof chunk === 'string' && output.length > 0 && output[output.length - 1] !== 10 + target.write(needsNewline ? Buffer.concat([output, Buffer.from('\n')]) : output, callback) + }, + }) } diff --git a/packages/app/src/cli/services/function/build.test.ts b/packages/app/src/cli/services/function/build.test.ts index 043ab427ec5..c1bebcd99cd 100644 --- a/packages/app/src/cli/services/function/build.test.ts +++ b/packages/app/src/cli/services/function/build.test.ts @@ -27,6 +27,8 @@ import {dirname, joinPath} from '@shopify/cli-kit/node/path' import {inTemporaryDirectory, mkdir, readFile, writeFile, removeFile} from '@shopify/cli-kit/node/fs' import {build as esBuild} from 'esbuild' import {runWithCommandEvents} from '@shopify/cli-kit/node/command-events' +import {acquireDevInstance} from '@shopify/cli-kit/node/dev-instance' +import {Writable} from 'stream' vi.mock('@shopify/cli-kit/node/system') vi.mock('@shopify/cli-kit/node/node-package-manager', async () => { @@ -478,6 +480,43 @@ describe('bundleExtension', () => { }) describe('runJavy', () => { + test('captures compiler diagnostics in the dev session log in text mode', async () => { + const {exec: execute} = + await vi.importActual('@shopify/cli-kit/node/system') + vi.mocked(exec).mockImplementation(async (_command, _args, options) => + execute( + process.execPath, + ['-e', "process.stderr.write('Javy: missing function export\\n'); process.exitCode = 1"], + options, + ), + ) + await inTemporaryDirectory(async (directory) => { + const fun = await testFunctionExtension({dir: directory}) + const instance = await acquireDevInstance({command: 'app dev', directory}) + let terminal = '' + const target = new Writable({ + write: (chunk, _encoding, done) => { + terminal += chunk.toString() + done() + }, + }) + const output = instance.tee(target, {source: 'discount', stringWritesAreMessages: true}) + try { + await expect(runJavy(fun, {stdout: output, stderr: output, app}, derivedDeps)).rejects.toThrow() + await instance.release() + expect(terminal).toContain('Javy: missing function export') + expect( + (await readFile(instance.paths.logFile)) + .trim() + .split('\n') + .map((line) => JSON.parse(line)), + ).toContainEqual(expect.objectContaining({source: 'discount', message: 'Javy: missing function export'})) + } finally { + await instance.release() + } + }) + }) + test('JSON context honors the supplied compiler sinks and signal', async () => { await inTemporaryDirectory(async (directory) => { const fun = await testFunctionExtension({dir: directory}) @@ -516,8 +555,8 @@ describe('runJavy', () => { ], { cwd: ourFunction.directory, - stderr: 'inherit', - stdout: 'inherit', + stderr, + stdout, signal, }, ) @@ -723,8 +762,8 @@ describe('ExportJavyBuilder', () => { ], { cwd: ourFunction.directory, - stderr: 'inherit', - stdout: 'inherit', + stderr, + stdout, signal, }, ) diff --git a/packages/app/src/cli/services/function/build.ts b/packages/app/src/cli/services/function/build.ts index e1d6e792356..57d09db0137 100644 --- a/packages/app/src/cli/services/function/build.ts +++ b/packages/app/src/cli/services/function/build.ts @@ -29,7 +29,6 @@ import { packageManagerBinaryCommandForDirectory, readAndParsePackageJson, } from '@shopify/cli-kit/node/node-package-manager' -import {commandEventOutputMode} from '@shopify/cli-kit/node/command-events' import {Writable} from 'stream' export const PREFERRED_FUNCTION_NPM_PACKAGE_MAJOR_VERSION = '2' @@ -406,8 +405,8 @@ export async function runJavy( return exec(javy.path, args, { cwd: fun.directory, - stdout: commandEventOutputMode() === 'json' ? options.stdout : 'inherit', - stderr: commandEventOutputMode() === 'json' ? options.stderr : 'inherit', + stdout: options.stdout, + stderr: options.stderr, signal: options.signal, }) } diff --git a/packages/app/test/fixtures/app-source-loader.js b/packages/app/test/fixtures/app-source-loader.js new file mode 100644 index 00000000000..c94cc9320d5 --- /dev/null +++ b/packages/app/test/fixtures/app-source-loader.js @@ -0,0 +1,15 @@ +const cliKitLoader = await import( + new URL('../../../cli-kit/test/fixtures/cli-kit-source-loader.js', import.meta.url).href +) + +// Unit-test CI has no generated workspace modules for these imports. +const workspaceSources = new Map([ + ['@shopify/organizations', '../../../organizations/src/index.js'], + ['@shopify/theme', '../../../theme/src/index.js'], +]) + +export function resolve(specifier, context, nextResolve) { + const source = workspaceSources.get(specifier) + if (source) return nextResolve(new URL(source, import.meta.url).href, context) + return cliKitLoader.resolve(specifier, context, nextResolve) +} diff --git a/packages/app/test/fixtures/tsconfig.json b/packages/app/test/fixtures/tsconfig.json new file mode 100644 index 00000000000..837f0441caf --- /dev/null +++ b/packages/app/test/fixtures/tsconfig.json @@ -0,0 +1,13 @@ +{ + "extends": "../../../../configurations/tsconfig.json", + "compilerOptions": { + "paths": { + "@shopify/cli-kit/*": ["../../../cli-kit/src/public/*"], + "@shopify/organizations": ["../../../organizations/src/index.ts"], + "@shopify/plugin-did-you-mean": ["../../../plugin-did-you-mean/src/index.ts"], + "@shopify/store": ["../../../store/src/index.ts"], + "@shopify/theme": ["../../../theme/src/index.ts"], + "@shopify/theme/*": ["../../../theme/src/*"] + } + } +} diff --git a/packages/cli-kit/src/public/node/tree-kill.test.ts b/packages/cli-kit/src/public/node/tree-kill.test.ts index b9111b26c32..36d1545dbe9 100644 --- a/packages/cli-kit/src/public/node/tree-kill.test.ts +++ b/packages/cli-kit/src/public/node/tree-kill.test.ts @@ -92,6 +92,25 @@ describe('treeKill', () => { }) }) + test('win32: preserves the root process when killRoot is false', async () => { + vi.stubGlobal('process', {...process, platform: 'win32'}) + vi.mocked(spawn).mockReturnValue(createMockProcess({exitCode: 0}) as any) + + await new Promise((resolve) => { + treeKill('1234', 'SIGINT', false, (error) => { + expect(error).toBeUndefined() + resolve() + }) + }) + + expect(spawn).toHaveBeenCalledWith('powershell.exe', [ + '-NoProfile', + '-NonInteractive', + '-Command', + 'Get-CimInstance Win32_Process -Filter "ParentProcessId = 1234 AND ProcessId <> $PID" -ErrorAction Stop | ForEach-Object { taskkill /pid $_.ProcessId /T /F | Out-Null }', + ]) + }) + test('darwin: recursively builds process tree and kills all processes', async () => { vi.stubGlobal('process', {...process, platform: 'darwin'}) const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => true) diff --git a/packages/cli-kit/src/public/node/tree-kill.ts b/packages/cli-kit/src/public/node/tree-kill.ts index dd651866e84..e77fcf6b1ee 100644 --- a/packages/cli-kit/src/public/node/tree-kill.ts +++ b/packages/cli-kit/src/public/node/tree-kill.ts @@ -70,12 +70,22 @@ function adaptedTreeKill( switch (process.platform) { case 'win32': { // Security: Use spawn instead of exec to avoid shell injection when killing processes on Windows - const taskkill = spawn('taskkill', ['/pid', rootPid, '/T', '/F']) + // Exclude the querying PowerShell process as well as the root when only children should stop. + const command = killRoot ? 'taskkill' : 'powershell.exe' + const args = killRoot + ? ['/pid', rootPid, '/T', '/F'] + : [ + '-NoProfile', + '-NonInteractive', + '-Command', + `Get-CimInstance Win32_Process -Filter "ParentProcessId = ${rootPid} AND ProcessId <> $PID" -ErrorAction Stop | ForEach-Object { taskkill /pid $_.ProcessId /T /F | Out-Null }`, + ] + const taskkill = spawn(command, args) taskkill.on('close', (code: number) => { if (code === 0) { callback() } else { - callback(new Error(`taskkill exited with code ${code}`)) + callback(new Error(`${command} exited with code ${code}`)) } }) break diff --git a/packages/cli/src/bootstrap.ts b/packages/cli/src/bootstrap.ts index 0eddb65e982..fb066d519d4 100644 --- a/packages/cli/src/bootstrap.ts +++ b/packages/cli/src/bootstrap.ts @@ -28,10 +28,15 @@ process.on('uncaughtException', async (err) => { await renderUncaughtError(err) process.exit(1) }) +const processEvents: NodeJS.EventEmitter = process +let shutdownRequested = false const signals = ['SIGINT', 'SIGTERM', 'SIGQUIT'] signals.forEach((signal) => { process.on(signal, () => { - process.exit(1) + // A repeated signal must not interrupt a command that is flushing its output. + if (shutdownRequested) return + shutdownRequested = processEvents.emit('shopify:signal', signal) + if (!shutdownRequested) process.exit(1) }) }) diff --git a/packages/cli/src/index.ts b/packages/cli/src/index.ts index a7a087bd4ce..dc7320892c5 100644 --- a/packages/cli/src/index.ts +++ b/packages/cli/src/index.ts @@ -59,10 +59,15 @@ process.on('uncaughtException', async (err) => { await renderUncaughtError(err) process.exit(1) }) +const processEvents: NodeJS.EventEmitter = process +let shutdownRequested = false const signals = ['SIGINT', 'SIGTERM', 'SIGQUIT'] signals.forEach((signal) => { process.on(signal, () => { - process.exit(1) + // A repeated signal must not interrupt a command that is flushing its output. + if (shutdownRequested) return + shutdownRequested = processEvents.emit('shopify:signal', signal) + if (!shutdownRequested) process.exit(1) }) }) diff --git a/patches/when-exit@2.1.5.patch b/patches/when-exit@2.1.5.patch new file mode 100644 index 00000000000..b7a51c5f530 --- /dev/null +++ b/patches/when-exit@2.1.5.patch @@ -0,0 +1,13 @@ +diff --git a/dist/node/interceptor.js b/dist/node/interceptor.js +index a0be92635ade17f6fd5ca75c441179d82e455dd9..72d1b4114cc80d6ae00b58c4b8198cfef9463b61 100644 +--- a/dist/node/interceptor.js ++++ b/dist/node/interceptor.js +@@ -17,7 +17,7 @@ class Interceptor { + for (const callback of this.callbacks) { + callback(); + } +- if (signal) { ++ if (signal && process.listenerCount(signal) === 0) { + if (IS_WINDOWS && (signal !== 'SIGINT' && signal !== 'SIGTERM' && signal !== 'SIGKILL')) { // Windows doesn't support POSIX signals, but Node emulates these 3 signals for us + process.kill(process.pid, 'SIGTERM'); + } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 274e667ca9d..8251ab9372a 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -34,6 +34,11 @@ overrides: nanoid: 3.3.8 graphql: 16.14.2 +patchedDependencies: + when-exit@2.1.5: + hash: dd8a5ad3c180e5b767f6050ad69222568dd4f0111de3769c24841d0caa8f26f7 + path: patches/when-exit@2.1.5.patch + importers: .: @@ -13399,7 +13404,7 @@ snapshots: atomically@2.1.1: dependencies: stubborn-fs: 2.0.0 - when-exit: 2.1.5 + when-exit: 2.1.5(patch_hash=dd8a5ad3c180e5b767f6050ad69222568dd4f0111de3769c24841d0caa8f26f7) auto-bind@4.0.0: {} @@ -18223,7 +18228,7 @@ snapshots: tr46: 5.1.1 webidl-conversions: 7.0.0 - when-exit@2.1.5: {} + when-exit@2.1.5(patch_hash=dd8a5ad3c180e5b767f6050ad69222568dd4f0111de3769c24841d0caa8f26f7): {} which-boxed-primitive@1.1.1: dependencies: