From f9bd5d46a49784cae338a794acccc2e10df9d9b3 Mon Sep 17 00:00:00 2001 From: Gonzalo Riestra Date: Mon, 20 Jul 2026 17:12:52 +0200 Subject: [PATCH 1/2] Send analytics in background so the main command can finish early --- .../src/private/node/otel-metrics.test.ts | 39 ++++ .../cli-kit/src/private/node/otel-metrics.ts | 5 +- .../cli-kit/src/public/node/analytics.test.ts | 179 +++++++++++++++++- packages/cli-kit/src/public/node/analytics.ts | 138 ++++++++++---- packages/cli-kit/src/public/node/fs.ts | 2 + .../cli-kit/src/public/node/hooks/postrun.ts | 4 +- .../public/node/notifications-system.test.ts | 4 +- .../src/public/node/notifications-system.ts | 5 +- .../cli-kit/src/public/node/system.test.ts | 55 +++++- packages/cli-kit/src/public/node/system.ts | 5 +- .../node/vendor/otel-js/service/types.ts | 3 +- packages/cli/oclif.manifest.json | 18 ++ .../cli/src/cli/commands/send-analytics.ts | 10 + packages/cli/src/index.ts | 2 + 14 files changed, 418 insertions(+), 51 deletions(-) create mode 100644 packages/cli/src/cli/commands/send-analytics.ts diff --git a/packages/cli-kit/src/private/node/otel-metrics.test.ts b/packages/cli-kit/src/private/node/otel-metrics.test.ts index 0209846d62a..8387dcc0329 100644 --- a/packages/cli-kit/src/private/node/otel-metrics.test.ts +++ b/packages/cli-kit/src/private/node/otel-metrics.test.ts @@ -26,11 +26,13 @@ describe('otel-metrics', () => { test('logs metrics when activated', async () => { const mockOtelRecorder = vi.fn() + const mockForceFlush = vi.fn().mockResolvedValue(undefined) const mockOtelCreator = vi.fn() mockOtelCreator.mockReturnValue({ type: 'otel', otel: { record: mockOtelRecorder, + getMeterProvider: () => ({forceFlush: mockForceFlush}), }, }) @@ -52,5 +54,42 @@ describe('otel-metrics', () => { expect(mockOtelCreator).toHaveBeenCalledOnce() expect(mockOtelRecorder.mock.calls).toMatchSnapshot() + expect(mockForceFlush).toHaveBeenCalledOnce() + }) + + test('waits for metrics to flush', async () => { + let resolveFlush: () => void = () => {} + const flush = new Promise((resolve) => { + resolveFlush = resolve + }) + const recorderFactory = vi.fn().mockReturnValue({ + type: 'otel', + otel: { + record: vi.fn(), + getMeterProvider: () => ({forceFlush: () => flush}), + }, + }) + + let metricsRecorded = false + const recording = recordMetrics( + { + skipMetricAnalytics: false, + cliVersion: '4.6.0', + owningPlugin: '@shopify/app', + command: 'app dev', + exitMode: 'ok', + }, + {active: 10, network: 20, prompt: 30}, + recorderFactory, + ).then(() => { + metricsRecorded = true + }) + + await vi.waitFor(() => expect(recorderFactory).toHaveBeenCalledOnce()) + expect(metricsRecorded).toBe(false) + + resolveFlush() + await recording + expect(metricsRecorded).toBe(true) }) }) diff --git a/packages/cli-kit/src/private/node/otel-metrics.ts b/packages/cli-kit/src/private/node/otel-metrics.ts index 5e0545503a9..ccef5e155ff 100644 --- a/packages/cli-kit/src/private/node/otel-metrics.ts +++ b/packages/cli-kit/src/private/node/otel-metrics.ts @@ -11,7 +11,7 @@ type MetricRecorder = | 'console' | { type: 'otel' - otel: Pick + otel: Pick } // this should be type, not interface @@ -80,6 +80,9 @@ export async function recordMetrics( recordCommandCounter(recorder, labels) recordCommandTiming(recorder, labels, timing) + if (recorder !== 'console') { + await recorder.otel.getMeterProvider().forceFlush({}) + } } const COMMAND_DURATION_BOUNDARIES_MS = [ diff --git a/packages/cli-kit/src/public/node/analytics.test.ts b/packages/cli-kit/src/public/node/analytics.test.ts index 61d8f67ca19..4cc3192d568 100644 --- a/packages/cli-kit/src/public/node/analytics.test.ts +++ b/packages/cli-kit/src/public/node/analytics.test.ts @@ -1,4 +1,11 @@ -import {reportAnalyticsEvent, recordTiming, recordError, recordRetry, recordEvent} from './analytics.js' +import { + reportAnalyticsEvent, + sendAnalyticsEventFromStdin, + recordTiming, + recordError, + recordRetry, + recordEvent, +} from './analytics.js' import * as os from './os.js' import { analyticsDisabled, @@ -16,11 +23,11 @@ import {mockAndCaptureOutput} from './testing/output.js' import {addPublicMetadata, addSensitiveMetadata} from './metadata.js' import {sendErrorToBugsnag} from './error-handler.js' import {hashString} from './crypto.js' +import {exec, readStdinString} from './system.js' import * as store from '../../private/node/analytics/storage.js' import {startAnalytics} from '../../private/node/analytics.js' import {CLI_KIT_VERSION} from '../common/version.js' import {setLastSeenAuthMethod, setLastSeenUserIdAfterAuth} from '../../private/node/session.js' - import {test, expect, describe, vi, beforeEach, afterEach, MockedFunction} from 'vitest' vi.mock('./context/local.js') @@ -32,6 +39,7 @@ vi.mock('../../version.js') vi.mock('./monorail.js') vi.mock('./cli.js') vi.mock('./error-handler.js') +vi.mock('./system.js') function restoreEnvVariable(key: string, value: string | undefined): void { if (value === undefined) { @@ -44,19 +52,21 @@ function restoreEnvVariable(key: string, value: string | undefined): void { describe('event tracking', () => { const currentDate = new Date(Date.UTC(2022, 1, 1, 10, 0, 0)) let publishEventMock: MockedFunction + let execMock: MockedFunction beforeEach(() => { vi.setSystemTime(currentDate) vi.mocked(isShopify).mockResolvedValue(false) vi.mocked(isDevelopment).mockReturnValue(false) vi.mocked(analyticsDisabled).mockReturnValue(false) - vi.mocked(ciPlatform).mockReturnValue({isCI: true, name: 'vitest', metadata: {}}) + vi.mocked(ciPlatform).mockReturnValue({isCI: false}) vi.mocked(macAddress).mockResolvedValue('macAddress') vi.mocked(hashString).mockReturnValue('hashed-macaddress') vi.mocked(isUnitTest).mockReturnValue(true) vi.mocked(cloudEnvironment).mockReturnValue({platform: 'localhost', editor: false}) vi.mocked(os.platformAndArch).mockReturnValue({platform: 'darwin', arch: 'arm64'}) publishEventMock = vi.mocked(publishMonorailEvent).mockReturnValue(Promise.resolve({type: 'ok'})) + execMock = vi.mocked(exec).mockResolvedValue(undefined) }) afterEach(() => { @@ -72,6 +82,163 @@ describe('event tracking', () => { }) } + async function sendReportedAnalyticsPayload(): Promise { + expect(execMock).toHaveBeenCalledOnce() + expect(execMock.mock.calls[0]![0]).toBe(process.execPath) + const execArgs = execMock.mock.calls[0]![1] + expect(execArgs.slice(1)).toEqual(['send-analytics']) + + const payloadInput = execMock.mock.calls[0]![2]?.input + if (payloadInput === undefined) throw new Error('Expected send-analytics to receive stdin input') + + vi.mocked(readStdinString).mockResolvedValueOnce(payloadInput) + await sendAnalyticsEventFromStdin() + } + + test('waits for the analytics process on Windows', async () => { + await inProjectWithFile('package.json', async (args) => { + // Given + const commandContent = {command: 'info', topic: 'app'} + await startAnalytics({commandContent, args, currentTime: currentDate.getTime() - 100}) + vi.mocked(os.platformAndArch).mockReturnValue({platform: 'windows', arch: 'arm64'}) + + let resolveAnalyticsProcess: () => void = () => {} + const analyticsProcess = new Promise((resolve) => { + resolveAnalyticsProcess = resolve + }) + execMock.mockReturnValueOnce(analyticsProcess) + + const config = { + runHook: vi.fn().mockResolvedValue({successes: [], failures: []}), + plugins: [], + } as any + + // When + let reportCompleted = false + const report = reportAnalyticsEvent({config, exitMode: 'expected_error'}).then(() => { + reportCompleted = true + }) + await vi.waitFor(() => expect(execMock).toHaveBeenCalledOnce()) + + // Then + expect(reportCompleted).toBe(false) + expect(execMock).toHaveBeenCalledWith( + expect.anything(), + expect.anything(), + expect.objectContaining({background: false}), + ) + resolveAnalyticsProcess() + await report + expect(reportCompleted).toBe(true) + await sendReportedAnalyticsPayload() + }) + }) + + test('does not wait for the analytics process on non-Windows platforms', async () => { + await inProjectWithFile('package.json', async (args) => { + // Given + const commandContent = {command: 'info', topic: 'app'} + await startAnalytics({commandContent, args, currentTime: currentDate.getTime() - 100}) + + let resolveAnalyticsProcess: () => void = () => {} + const analyticsProcess = new Promise((resolve) => { + resolveAnalyticsProcess = resolve + }) + execMock.mockReturnValueOnce(analyticsProcess) + + const config = { + runHook: vi.fn().mockResolvedValue({successes: [], failures: []}), + plugins: [], + } as any + + // When + await reportAnalyticsEvent({config, exitMode: 'expected_error'}) + + // Then + expect(execMock).toHaveBeenCalledWith( + expect.anything(), + expect.anything(), + expect.objectContaining({background: true, input: expect.any(String)}), + ) + resolveAnalyticsProcess() + await sendReportedAnalyticsPayload() + }) + }) + + test('waits for the analytics process in CI', async () => { + await inProjectWithFile('package.json', async (args) => { + // Given + const commandContent = {command: 'info', topic: 'app'} + await startAnalytics({commandContent, args, currentTime: currentDate.getTime() - 100}) + vi.mocked(ciPlatform).mockReturnValue({isCI: true, name: 'github', metadata: {}}) + + let resolveAnalyticsProcess: () => void = () => {} + const analyticsProcess = new Promise((resolve) => { + resolveAnalyticsProcess = resolve + }) + execMock.mockReturnValueOnce(analyticsProcess) + + const config = { + runHook: vi.fn().mockResolvedValue({successes: [], failures: []}), + plugins: [], + } as any + + // When + let reportCompleted = false + const report = reportAnalyticsEvent({config, exitMode: 'ok'}).then(() => { + reportCompleted = true + }) + await vi.waitFor(() => expect(execMock).toHaveBeenCalledOnce()) + + // Then + expect(reportCompleted).toBe(false) + expect(execMock).toHaveBeenCalledWith( + process.execPath, + expect.anything(), + expect.objectContaining({background: false}), + ) + resolveAnalyticsProcess() + await report + expect(reportCompleted).toBe(true) + await sendReportedAnalyticsPayload() + }) + }) + + test('sends analytics synchronously for create-app', async () => { + await inProjectWithFile('package.json', async (args) => { + // Given + const commandContent = {command: 'init'} + await startAnalytics({commandContent, args, currentTime: currentDate.getTime() - 100}) + const config = { + bin: 'create-app', + runHook: vi.fn().mockResolvedValue({successes: [], failures: []}), + plugins: [], + } as any + + // When + await reportAnalyticsEvent({config, exitMode: 'ok'}) + + // Then + expect(execMock).not.toHaveBeenCalled() + expect(publishEventMock).toHaveBeenCalledOnce() + }) + }) + + test('reports invalid analytics JSON received from stdin', async () => { + // Given + vi.mocked(readStdinString).mockResolvedValueOnce('{invalid') + const outputMock = mockAndCaptureOutput() + + // When + await sendAnalyticsEventFromStdin() + + // Then + expect(outputMock.debug()).toContain('Failed to send analytics in background') + expect(publishEventMock).not.toHaveBeenCalled() + expect(sendErrorToBugsnag).toHaveBeenCalledOnce() + expect(sendErrorToBugsnag).toHaveBeenCalledWith(expect.any(Error), 'expected_error') + }) + test('sends the expected data to Monorail with cached app info', async () => { await inProjectWithFile('package.json', async (args) => { // Given @@ -95,6 +262,7 @@ describe('event tracking', () => { plugins: pluginsMap, } as any await reportAnalyticsEvent({config, exitMode: 'ok'}) + await sendReportedAnalyticsPayload() // Then const version = CLI_KIT_VERSION const expectedPayloadPublic = { @@ -155,6 +323,7 @@ describe('event tracking', () => { plugins: [], } as any await reportAnalyticsEvent({config, exitMode: 'ok'}) + await sendReportedAnalyticsPayload() // Then expect(publishEventMock).toHaveBeenCalledOnce() @@ -179,6 +348,7 @@ describe('event tracking', () => { plugins: [], } as any await reportAnalyticsEvent({config, errorMessage: 'Permission denied', exitMode: 'unexpected_error'}) + await sendReportedAnalyticsPayload() // Then const version = CLI_KIT_VERSION @@ -219,6 +389,7 @@ describe('event tracking', () => { plugins: [], } as any await reportAnalyticsEvent({config, exitMode: 'ok'}) + await sendReportedAnalyticsPayload() // Then const expectedPayloadSensitive = { @@ -243,6 +414,7 @@ describe('event tracking', () => { plugins: [], } as any await reportAnalyticsEvent({config, exitMode: 'ok'}) + await sendReportedAnalyticsPayload() expect(publishEventMock).toHaveBeenCalledOnce() expect(publishEventMock.mock.calls[0]![2]).toMatchObject({ @@ -274,6 +446,7 @@ describe('event tracking', () => { plugins: [], } as any await reportAnalyticsEvent({config, exitMode: 'ok'}) + await sendReportedAnalyticsPayload() // Then const sensitivePayload = publishEventMock.mock.calls[0]![2] diff --git a/packages/cli-kit/src/public/node/analytics.ts b/packages/cli-kit/src/public/node/analytics.ts index dcaef13d51c..10eff69b995 100644 --- a/packages/cli-kit/src/public/node/analytics.ts +++ b/packages/cli-kit/src/public/node/analytics.ts @@ -1,6 +1,6 @@ -import {alwaysLogAnalytics, alwaysLogMetrics, analyticsDisabled, isShopify} from './context/local.js' +import {alwaysLogAnalytics, alwaysLogMetrics, analyticsDisabled, ciPlatform, isShopify} from './context/local.js' import * as metadata from './metadata.js' -import {publishMonorailEvent, MONORAIL_COMMAND_TOPIC} from './monorail.js' +import {publishMonorailEvent, MONORAIL_COMMAND_TOPIC, type Schemas} from './monorail.js' import {fanoutHooks} from './plugins.js' import {sendErrorToBugsnag} from './error-handler.js' import {outputContent, outputDebug, outputToken} from './output.js' @@ -36,6 +36,70 @@ interface ReportAnalyticsEventOptions { exitMode: CommandExitMode } +type MonorailCommandPayload = Schemas[typeof MONORAIL_COMMAND_TOPIC] +interface AnalyticsPayload { + public: MonorailCommandPayload['public'] & {cmd_all_exit: CommandExitMode} + sensitive: MonorailCommandPayload['sensitive'] +} + +async function sendAnalyticsEvent( + payload: AnalyticsPayload, + skipMonorailAnalytics: boolean, + skipMetricAnalytics: boolean, +): Promise { + const doMonorail = async () => { + if (skipMonorailAnalytics) return + const response = await publishMonorailEvent(MONORAIL_COMMAND_TOPIC, payload.public, payload.sensitive) + if (response.type === 'error') { + outputDebug(response.message) + } + } + + const doOpenTelemetry = async () => { + const active = payload.public.cmd_all_timing_active_ms ?? 0 + const network = payload.public.cmd_all_timing_network_ms ?? 0 + const prompt = payload.public.cmd_all_timing_prompts_ms ?? 0 + + return recordMetrics( + { + skipMetricAnalytics, + cliVersion: payload.public.cli_version, + owningPlugin: payload.public.cmd_all_plugin ?? '@shopify/cli', + command: payload.public.command, + exitMode: payload.public.cmd_all_exit, + }, + { + active, + network, + prompt, + }, + ) + } + + await Promise.all([doMonorail(), doOpenTelemetry()]) +} + +export async function sendAnalyticsEventFromStdin(): Promise { + try { + const {readStdinString} = await import('./system.js') + const payloadStr = await readStdinString() + if (payloadStr === undefined) throw new Error('No analytics payload received from stdin') + + const {payload, skipMonorailAnalytics, skipMetricAnalytics} = JSON.parse(payloadStr) as { + payload: AnalyticsPayload + skipMonorailAnalytics: boolean + skipMetricAnalytics: boolean + } + + await sendAnalyticsEvent(payload, skipMonorailAnalytics, skipMetricAnalytics) + // eslint-disable-next-line no-catch-all/no-catch-all + } catch (error) { + const message = error instanceof Error ? error.message : String(error) + outputDebug(`Failed to send analytics in background: ${message}`) + await sendErrorToBugsnag(error, 'expected_error') + } +} + /** * Report an analytics event, sending it off to Monorail -- Shopify's internal analytics service. * @@ -45,8 +109,7 @@ interface ReportAnalyticsEventOptions { export async function reportAnalyticsEvent(options: ReportAnalyticsEventOptions): Promise { try { const payload = await buildPayload(options) - if (payload === undefined) { - // Nothing to log + if (payload === undefined || payload.public.command === 'send-analytics') { return } @@ -65,40 +128,43 @@ export async function reportAnalyticsEvent(options: ReportAnalyticsEventOptions) const skipMonorailAnalytics = !alwaysLogAnalytics() && analyticsDisabled() const skipMetricAnalytics = !alwaysLogMetrics() && analyticsDisabled() - if (skipMonorailAnalytics || skipMetricAnalytics) { + if (skipMonorailAnalytics && skipMetricAnalytics) { outputDebug(outputContent`Skipping command analytics, payload: ${outputToken.json(payload)}`) + return } - const doMonorail = async () => { - if (skipMonorailAnalytics) { - return - } - const response = await publishMonorailEvent(MONORAIL_COMMAND_TOPIC, payload.public, payload.sensitive) - if (response.type === 'error') { - outputDebug(response.message) - } + const {platformAndArch} = await import('./os.js') + const sendSynchronously = options.config.bin === 'create-app' + const sendInBackground = !sendSynchronously && platformAndArch().platform !== 'windows' && !ciPlatform().isCI + const deliveryDescription = sendInBackground ? ' in background' : '' + outputDebug(outputContent`Sending command analytics${deliveryDescription}, payload: ${outputToken.json(payload)}`) + + if (sendSynchronously) { + await sendAnalyticsEvent(payload, skipMonorailAnalytics, skipMetricAnalytics) + return } - const doOpenTelemetry = async () => { - const active = payload.public.cmd_all_timing_active_ms ?? 0 - const network = payload.public.cmd_all_timing_network_ms ?? 0 - const prompt = payload.public.cmd_all_timing_prompts_ms ?? 0 - - return recordMetrics( - { - skipMetricAnalytics, - cliVersion: payload.public.cli_version, - owningPlugin: payload.public.cmd_all_plugin ?? '@shopify/cli', - command: payload.public.command, - exitMode: options.exitMode, - }, - { - active, - network, - prompt, - }, - ) + + const {exec} = await import('./system.js') + const argv = process.argv + if (!argv[1]) return + const nodeBinary = process.execPath + const shopifyBinary = argv[1] + const args = [shopifyBinary, 'send-analytics'] + + const analyticsProcess = exec(nodeBinary, args, { + background: sendInBackground, + env: {...process.env, SHOPIFY_CLI_NO_ANALYTICS: '1'}, + input: JSON.stringify({payload, skipMonorailAnalytics, skipMetricAnalytics}), + externalErrorHandler: async (error: unknown) => { + outputDebug(`Failed to send analytics in background: ${(error as Error).message}`) + }, + }) + if (sendInBackground) { + // eslint-disable-next-line no-void + void analyticsProcess + } else { + await analyticsProcess } - await Promise.all([doMonorail(), doOpenTelemetry()]) // eslint-disable-next-line no-catch-all/no-catch-all } catch (error) { @@ -111,7 +177,11 @@ export async function reportAnalyticsEvent(options: ReportAnalyticsEventOptions) } } -async function buildPayload({config, errorMessage, exitMode}: ReportAnalyticsEventOptions) { +async function buildPayload({ + config, + errorMessage, + exitMode, +}: ReportAnalyticsEventOptions): Promise { const {commandStartOptions, environmentFlags, ...sensitiveMetadata} = metadata.getAllSensitiveMetadata() if (commandStartOptions === undefined) { outputDebug('Unable to log analytics event - no information on executed command') diff --git a/packages/cli-kit/src/public/node/fs.ts b/packages/cli-kit/src/public/node/fs.ts index 5948c9f6bb6..6cade41e992 100644 --- a/packages/cli-kit/src/public/node/fs.ts +++ b/packages/cli-kit/src/public/node/fs.ts @@ -211,6 +211,8 @@ export function appendFileSync(path: string, data: string): void { export interface WriteOptions { encoding: BufferEncoding + mode?: number + flag?: string } /** diff --git a/packages/cli-kit/src/public/node/hooks/postrun.ts b/packages/cli-kit/src/public/node/hooks/postrun.ts index 8718e50218e..08d2afd6518 100644 --- a/packages/cli-kit/src/public/node/hooks/postrun.ts +++ b/packages/cli-kit/src/public/node/hooks/postrun.ts @@ -74,11 +74,11 @@ export const hook: Hook.Postrun = async ({config, Command}) => { const command = Command.id.replace(/:/g, ' ') outputDebug(`Completed command ${command}`) - if (!command.includes('notifications') && !command.includes('upgrade')) await autoUpgradeIfNeeded() + if (!command.includes('notifications') && !command.includes('upgrade') && !command.includes('send-analytics')) + await autoUpgradeIfNeeded() const {reportAnalyticsEvent} = await import('../analytics.js') await reportAnalyticsEvent({config, exitMode: 'ok'}) - postRunHookCompleted = true } diff --git a/packages/cli-kit/src/public/node/notifications-system.test.ts b/packages/cli-kit/src/public/node/notifications-system.test.ts index 1eefbe57a37..4ce2f8b0761 100644 --- a/packages/cli-kit/src/public/node/notifications-system.test.ts +++ b/packages/cli-kit/src/public/node/notifications-system.test.ts @@ -442,7 +442,7 @@ describe('fetchNotificationsInBackground', () => { expect(exec).not.toHaveBeenCalled() }) - test('calls the expected Shopify binary', async () => { + test('calls the current Shopify entry point with the canonical Node executable', async () => { // Given / When fetchNotificationsInBackground('theme:list', ['/path/to/node', '/path/to/shopify', 'theme', 'list'], { SHOPIFY_UNIT_TEST: 'false', @@ -450,7 +450,7 @@ describe('fetchNotificationsInBackground', () => { // Then expect(exec).toHaveBeenCalledWith( - '/path/to/node', + process.execPath, ['/path/to/shopify', 'notifications', 'list', '--ignore-errors'], expect.anything(), ) diff --git a/packages/cli-kit/src/public/node/notifications-system.ts b/packages/cli-kit/src/public/node/notifications-system.ts index d9eade1f1cb..521b427a24a 100644 --- a/packages/cli-kit/src/public/node/notifications-system.ts +++ b/packages/cli-kit/src/public/node/notifications-system.ts @@ -21,6 +21,7 @@ const COMMANDS_TO_SKIP = [ 'theme:init', 'hydrogen:init', 'cache:clear', + 'send-analytics', ] function url(): string { @@ -179,10 +180,10 @@ export function fetchNotificationsInBackground( environment: NodeJS.ProcessEnv = process.env, ): void { if (skipNotifications(currentCommand, environment)) return - if (!argv[0] || !argv[1]) return + if (!argv[1]) return // Run the Shopify command the same way as the current execution - const nodeBinary = argv[0] + const nodeBinary = process.execPath const shopifyBinary = argv[1] const args = [shopifyBinary, 'notifications', 'list', '--ignore-errors'] diff --git a/packages/cli-kit/src/public/node/system.test.ts b/packages/cli-kit/src/public/node/system.test.ts index 6c863f60dd4..3c5591926de 100644 --- a/packages/cli-kit/src/public/node/system.test.ts +++ b/packages/cli-kit/src/public/node/system.test.ts @@ -276,6 +276,28 @@ describe('execCommand', () => { expect(execa).toHaveBeenCalledWith('cat', [], expect.objectContaining({stdin: 'inherit'})) }) + test.skipIf(process.platform === 'win32')( + 'pipes input to a background process while ignoring its output', + async () => { + // Given + vi.mocked(which.sync).mockReturnValueOnce('/system/cat') + const unref = vi.fn() + const childProcess = Object.assign(Promise.resolve({}), {unref}) + vi.mocked(execa).mockReturnValueOnce(childProcess as any) + + // When + await system.exec('cat', [], {background: true, input: 'payload'}) + + // Then + expect(execa).toHaveBeenCalledWith( + 'cat', + [], + expect.objectContaining({input: 'payload', stdio: ['pipe', 'ignore', 'ignore']}), + ) + expect(unref).toHaveBeenCalledOnce() + }, + ) + test('raises an error if the command to run is found in the current directory', async () => { // Given vi.mocked(which.sync).mockReturnValueOnce('/currentDirectory/command') @@ -308,7 +330,11 @@ describe('execCommand', () => { describe('isStdinPiped', () => { test('returns true when stdin is a FIFO (pipe)', () => { // Given - vi.mocked(fs.fstatSync).mockReturnValue({isFIFO: () => true, isFile: () => false} as fs.Stats) + vi.mocked(fs.fstatSync).mockReturnValue({ + isFIFO: () => true, + isFile: () => false, + isSocket: () => false, + } as fs.Stats) // When const got = system.isStdinPiped() @@ -319,7 +345,26 @@ describe('isStdinPiped', () => { test('returns true when stdin is a file redirect', () => { // Given - vi.mocked(fs.fstatSync).mockReturnValue({isFIFO: () => false, isFile: () => true} as fs.Stats) + vi.mocked(fs.fstatSync).mockReturnValue({ + isFIFO: () => false, + isFile: () => true, + isSocket: () => false, + } as fs.Stats) + + // When + const got = system.isStdinPiped() + + // Then + expect(got).toBe(true) + }) + + test('returns true when stdin is a child-process pipe represented as a socket', () => { + // Given + vi.mocked(fs.fstatSync).mockReturnValue({ + isFIFO: () => false, + isFile: () => false, + isSocket: () => true, + } as fs.Stats) // When const got = system.isStdinPiped() @@ -330,7 +375,11 @@ describe('isStdinPiped', () => { test('returns false when stdin is a TTY (interactive)', () => { // Given - vi.mocked(fs.fstatSync).mockReturnValue({isFIFO: () => false, isFile: () => false} as fs.Stats) + vi.mocked(fs.fstatSync).mockReturnValue({ + isFIFO: () => false, + isFile: () => false, + isSocket: () => false, + } as fs.Stats) // When const got = system.isStdinPiped() diff --git a/packages/cli-kit/src/public/node/system.ts b/packages/cli-kit/src/public/node/system.ts index aa997f8c5d8..1c36b1b527f 100644 --- a/packages/cli-kit/src/public/node/system.ts +++ b/packages/cli-kit/src/public/node/system.ts @@ -277,11 +277,12 @@ function buildExec( } const executionCwd = options?.cwd ?? cwd() checkCommandSafety(command, {cwd: executionCwd}) + const backgroundStdio = options?.input === undefined ? 'ignore' : (['pipe', 'ignore', 'ignore'] as const) const commandProcess = execa(command, args, { env, cwd: executionCwd, input: options?.input, - stdio: options?.background ? 'ignore' : options?.stdio, + stdio: options?.background ? backgroundStdio : options?.stdio, stdin: options?.stdin, stdout: options?.stdout === 'inherit' ? 'inherit' : undefined, stderr: options?.stderr === 'inherit' ? 'inherit' : undefined, @@ -373,7 +374,7 @@ export async function isWsl(): Promise { export function isStdinPiped(): boolean { try { const stats = fstatSync(0) - return stats.isFIFO() || stats.isFile() + return stats.isFIFO() || stats.isFile() || stats.isSocket() // eslint-disable-next-line no-catch-all/no-catch-all } catch { return false diff --git a/packages/cli-kit/src/public/node/vendor/otel-js/service/types.ts b/packages/cli-kit/src/public/node/vendor/otel-js/service/types.ts index dde478922cc..0c724d2d0f0 100644 --- a/packages/cli-kit/src/public/node/vendor/otel-js/service/types.ts +++ b/packages/cli-kit/src/public/node/vendor/otel-js/service/types.ts @@ -1,12 +1,11 @@ import type { Counter, Histogram, - MeterProvider, MetricAttributes, MetricOptions, UpDownCounter, } from '@opentelemetry/api' -import type {ViewOptions} from '@opentelemetry/sdk-metrics' +import type {MeterProvider, ViewOptions} from '@opentelemetry/sdk-metrics' export type CustomMetricLabels< TLabels extends Record, diff --git a/packages/cli/oclif.manifest.json b/packages/cli/oclif.manifest.json index 98c1bcf5ac5..45ea376b354 100644 --- a/packages/cli/oclif.manifest.json +++ b/packages/cli/oclif.manifest.json @@ -6314,6 +6314,24 @@ "strict": true, "usage": "search [query]" }, + "send-analytics": { + "aliases": [ + ], + "args": { + }, + "enableJsonFlag": false, + "flags": { + }, + "hasDynamicHelp": false, + "hidden": true, + "hiddenAliases": [ + ], + "id": "send-analytics", + "pluginAlias": "@shopify/cli", + "pluginName": "@shopify/cli", + "pluginType": "core", + "strict": true + }, "store:auth": { "aliases": [ ], diff --git a/packages/cli/src/cli/commands/send-analytics.ts b/packages/cli/src/cli/commands/send-analytics.ts new file mode 100644 index 00000000000..d60db23b683 --- /dev/null +++ b/packages/cli/src/cli/commands/send-analytics.ts @@ -0,0 +1,10 @@ +import Command from '@shopify/cli-kit/node/base-command' +import {sendAnalyticsEventFromStdin} from '@shopify/cli-kit/node/analytics' + +export default class SendAnalytics extends Command { + static hidden = true + + async run(): Promise { + await sendAnalyticsEventFromStdin() + } +} diff --git a/packages/cli/src/index.ts b/packages/cli/src/index.ts index 7651e66d514..027585eac79 100644 --- a/packages/cli/src/index.ts +++ b/packages/cli/src/index.ts @@ -1,6 +1,7 @@ import VersionCommand from './cli/commands/version.js' import Search from './cli/commands/search.js' import Upgrade from './cli/commands/upgrade.js' +import SendAnalytics from './cli/commands/send-analytics.js' import Logout from './cli/commands/auth/logout.js' import Login from './cli/commands/auth/login.js' import CommandFlags from './cli/commands/debug/command-flags.js' @@ -149,6 +150,7 @@ export const COMMANDS: any = { search: Search, upgrade: Upgrade, version: VersionCommand, + 'send-analytics': SendAnalytics, help: HelpCommand, 'auth:logout': Logout, 'auth:login': Login, From 8fb2bd418db444bc3876eba210ca7c1e91ed067e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Mon, 10 Aug 2026 14:03:02 +0200 Subject: [PATCH 2/2] Lazy-load app context in public_command_metadata hook MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The analytics postrun hook eagerly imported app-context.js, pulling in a large module graph on every command — ~150-190ms of buildPayload time even for commands like 'shopify version' outside an app project. The hook now checks for a shopify.app*.toml in the directory tree before dynamically importing the app context, so commands outside an app project skip the import entirely. Co-Authored-By: Claude Fable 5 --- packages/app/src/cli/constants.ts | 3 + .../app/src/cli/hooks/public_metadata.test.ts | 84 +++++++++++++------ packages/app/src/cli/hooks/public_metadata.ts | 20 ++++- .../app/src/cli/models/project/project.ts | 4 +- 4 files changed, 82 insertions(+), 29 deletions(-) diff --git a/packages/app/src/cli/constants.ts b/packages/app/src/cli/constants.ts index d5ffe02e06e..e7db8b82350 100644 --- a/packages/app/src/cli/constants.ts +++ b/packages/app/src/cli/constants.ts @@ -8,6 +8,9 @@ export const environmentVariableNames = { disableMinificationOnDev: 'SHOPIFY_CLI_DISABLE_MINIFICATION_ON_DEV', } +// Matches the default config file (shopify.app.toml) and named configs (shopify.app..toml) +export const appConfigurationFileGlob = 'shopify.app*.toml' + export const configurationFileNames = { app: 'shopify.app.toml', web: 'shopify.web.toml', diff --git a/packages/app/src/cli/hooks/public_metadata.test.ts b/packages/app/src/cli/hooks/public_metadata.test.ts index 1b52352b938..6df08bc4d47 100644 --- a/packages/app/src/cli/hooks/public_metadata.test.ts +++ b/packages/app/src/cli/hooks/public_metadata.test.ts @@ -2,51 +2,85 @@ import gatherPublicMetadata from './public_metadata.js' import {localAppContext} from '../services/app-context.js' import metadata from '../metadata.js' import {describe, expect, test, vi, beforeEach} from 'vitest' -import {cwd} from '@shopify/cli-kit/node/path' +import {cwd, joinPath} from '@shopify/cli-kit/node/path' +import {inTemporaryDirectory, writeFile} from '@shopify/cli-kit/node/fs' vi.mock('../services/app-context.js') -vi.mock('@shopify/cli-kit/node/path') +vi.mock('@shopify/cli-kit/node/path', async (importOriginal) => { + const actual = await importOriginal() + return {...actual, cwd: vi.fn()} +}) + +async function inTemporaryAppProject(runTest: (appDirectory: string) => Promise): Promise { + await inTemporaryDirectory(async (tmpDir) => { + await writeFile(joinPath(tmpDir, 'shopify.app.toml'), '') + await runTest(tmpDir) + }) +} describe('gatherPublicMetadata', () => { beforeEach(() => { - vi.mocked(cwd).mockReturnValue('/some/app/dir') vi.mocked(localAppContext).mockResolvedValue({} as Awaited>) }) test('opportunistically enriches metadata from the current directory and returns the public metadata', async () => { - // Given - vi.spyOn(metadata, 'getAllPublicMetadata').mockReturnValueOnce({}).mockReturnValue({api_key: 'from-loader'}) + await inTemporaryAppProject(async (appDirectory) => { + // Given + vi.mocked(cwd).mockReturnValue(appDirectory) + vi.spyOn(metadata, 'getAllPublicMetadata').mockReturnValueOnce({}).mockReturnValue({api_key: 'from-loader'}) - // When - const result = await (gatherPublicMetadata as () => Promise)() + // When + const result = await (gatherPublicMetadata as () => Promise)() - // Then - expect(localAppContext).toHaveBeenCalledWith({directory: '/some/app/dir', skipPrompts: true}) - expect(result).toEqual(metadata.getAllPublicMetadata()) + // Then + expect(localAppContext).toHaveBeenCalledWith({directory: appDirectory, skipPrompts: true}) + expect(result).toEqual(metadata.getAllPublicMetadata()) + }) }) test('skips local app loading when api_key is already set', async () => { - // Given - vi.spyOn(metadata, 'getAllPublicMetadata').mockReturnValue({api_key: 'already-set'}) + await inTemporaryAppProject(async (appDirectory) => { + // Given + vi.mocked(cwd).mockReturnValue(appDirectory) + vi.spyOn(metadata, 'getAllPublicMetadata').mockReturnValue({api_key: 'already-set'}) + + // When + const result = await (gatherPublicMetadata as () => Promise)() + + // Then + expect(localAppContext).not.toHaveBeenCalled() + expect(result).toEqual(metadata.getAllPublicMetadata()) + }) + }) + + test('skips local app loading when the directory is not inside an app project', async () => { + await inTemporaryDirectory(async (tmpDir) => { + // Given + vi.mocked(cwd).mockReturnValue(tmpDir) + vi.spyOn(metadata, 'getAllPublicMetadata').mockReturnValue({}) - // When - const result = await (gatherPublicMetadata as () => Promise)() + // When + const result = await (gatherPublicMetadata as () => Promise)() - // Then - expect(localAppContext).not.toHaveBeenCalled() - expect(result).toEqual(metadata.getAllPublicMetadata()) + // Then + expect(localAppContext).not.toHaveBeenCalled() + expect(result).toEqual(metadata.getAllPublicMetadata()) + }) }) test('still returns metadata when best-effort app loading fails', async () => { - // Given - vi.spyOn(metadata, 'getAllPublicMetadata').mockReturnValue({}) - vi.mocked(localAppContext).mockRejectedValue(new Error('not an app')) + await inTemporaryAppProject(async (appDirectory) => { + // Given + vi.mocked(cwd).mockReturnValue(appDirectory) + vi.spyOn(metadata, 'getAllPublicMetadata').mockReturnValue({}) + vi.mocked(localAppContext).mockRejectedValue(new Error('not an app')) - // When - const result = await (gatherPublicMetadata as () => Promise)() + // When + const result = await (gatherPublicMetadata as () => Promise)() - // Then - expect(localAppContext).toHaveBeenCalledOnce() - expect(result).toEqual(metadata.getAllPublicMetadata()) + // Then + expect(localAppContext).toHaveBeenCalledOnce() + expect(result).toEqual(metadata.getAllPublicMetadata()) + }) }) }) diff --git a/packages/app/src/cli/hooks/public_metadata.ts b/packages/app/src/cli/hooks/public_metadata.ts index 03b0a8bc7b2..6a3d463aad4 100644 --- a/packages/app/src/cli/hooks/public_metadata.ts +++ b/packages/app/src/cli/hooks/public_metadata.ts @@ -1,15 +1,31 @@ import metadata from '../metadata.js' -import {localAppContext} from '../services/app-context.js' +import {appConfigurationFileGlob} from '../constants.js' import {FanoutHookFunction} from '@shopify/cli-kit/node/plugins' -import {cwd} from '@shopify/cli-kit/node/path' +import {cwd, joinPath} from '@shopify/cli-kit/node/path' +import {findPathUp, glob} from '@shopify/cli-kit/node/fs' const APP_CONTEXT_METADATA_TIMEOUT_MS = 3000 +async function insideAppProject(directory: string): Promise { + const found = await findPathUp( + async (candidateDirectory) => { + const matches = await glob(joinPath(candidateDirectory, appConfigurationFileGlob)) + if (matches.length > 0) return candidateDirectory + }, + {cwd: directory, type: 'directory'}, + ) + return found !== undefined +} + async function logAppContextMetadata(directory: string): Promise { let timer: ReturnType | undefined try { if (metadata.getAllPublicMetadata().api_key !== undefined) return + if (!(await insideAppProject(directory))) return + // Loading the app context pulls in a large module graph, so only import it + // once we know the command ran inside an app project. + const {localAppContext} = await import('../services/app-context.js') await Promise.race([ localAppContext({directory, skipPrompts: true}), new Promise((resolve) => { diff --git a/packages/app/src/cli/models/project/project.ts b/packages/app/src/cli/models/project/project.ts index f34ac61bedb..f0055c80b09 100644 --- a/packages/app/src/cli/models/project/project.ts +++ b/packages/app/src/cli/models/project/project.ts @@ -1,4 +1,4 @@ -import {configurationFileNames} from '../../constants.js' +import {appConfigurationFileGlob, configurationFileNames} from '../../constants.js' import {TomlFile, TomlFileError} from '@shopify/cli-kit/node/toml/toml-file' import {readAndParseDotEnv, DotEnvFile} from '@shopify/cli-kit/node/dot-env' import {fileExists, glob, findPathUp, readFile} from '@shopify/cli-kit/node/fs' @@ -12,7 +12,7 @@ import {joinPath, basename} from '@shopify/cli-kit/node/path' import {AbortError} from '@shopify/cli-kit/node/error' import {JsonMapType} from '@shopify/cli-kit/node/toml' -const APP_CONFIG_GLOB = 'shopify.app*.toml' +const APP_CONFIG_GLOB = appConfigurationFileGlob const APP_CONFIG_REGEX = /^shopify\.app(\.[-\w]+)?\.toml$/ const EXTENSION_TOML = '*.extension.toml' const WEB_TOML = 'shopify.web.toml'