diff --git a/.changeset/quiet-prompt-streams.md b/.changeset/quiet-prompt-streams.md new file mode 100644 index 00000000000..ca1bcff582c --- /dev/null +++ b/.changeset/quiet-prompt-streams.md @@ -0,0 +1,6 @@ +--- +'@shopify/cli': patch +'@shopify/cli-kit': patch +--- + +Write interactive prompts to stderr by default to keep stdout clear for command results. diff --git a/.changeset/strict-live-theme-confirmation.md b/.changeset/strict-live-theme-confirmation.md new file mode 100644 index 00000000000..b536a3ca6bf --- /dev/null +++ b/.changeset/strict-live-theme-confirmation.md @@ -0,0 +1,5 @@ +--- +'@shopify/cli': patch +--- + +Require --allow-live for theme dev on a live theme when prompting is unavailable. diff --git a/docs/cli-kit/ui-kit/readme.md b/docs/cli-kit/ui-kit/readme.md index 1b2bcdd399d..7b4be4f94d8 100644 --- a/docs/cli-kit/ui-kit/readme.md +++ b/docs/cli-kit/ui-kit/readme.md @@ -153,6 +153,8 @@ when you want to display it to the terminal, for example in your exception handl Prompts interrupt the flow of commands to ask the user for some information. There are two main types of prompts: selects and textual prompts. They all take a `message` property which will be displayed as the title of the prompt to the user and is usually in the form of a question. If you forget to use punctuation at the end of `message` we'll add `?` for you. +Prompts read input from stdin and write to stderr by default, so stdout can contain command results. By default, both stdin and stderr must be terminals for prompting. To use a different output stream, pass it as `renderOptions.stdout`. + For select prompts, if the terminal is not tall enough to render all the options the prompt will resize to fit the terminal window and a message will appear warning the user that only a fraction of the items is being displayed. The user can then cycle through the items, including the hidden ones, by using the arrow keys. #### `renderSelectPrompt` diff --git a/packages/app/src/cli/commands/app/init.test.ts b/packages/app/src/cli/commands/app/init.test.ts index 90c1d013279..97a95ffe4db 100644 --- a/packages/app/src/cli/commands/app/init.test.ts +++ b/packages/app/src/cli/commands/app/init.test.ts @@ -36,20 +36,20 @@ vi.mock('../../services/init/validate.js') vi.mock('@shopify/cli-kit/node/node-package-manager') let originalStdinIsTTY: boolean | undefined -let originalStdoutIsTTY: boolean | undefined +let originalStderrIsTTY: boolean | undefined beforeEach(() => { originalStdinIsTTY = process.stdin.isTTY - originalStdoutIsTTY = process.stdout.isTTY + originalStderrIsTTY = process.stderr.isTTY vi.unstubAllEnvs() Object.defineProperty(process.stdin, 'isTTY', {value: true, configurable: true, writable: true}) - Object.defineProperty(process.stdout, 'isTTY', {value: true, configurable: true, writable: true}) + Object.defineProperty(process.stderr, 'isTTY', {value: true, configurable: true, writable: true}) vi.stubEnv('CI', '') }) afterEach(() => { Object.defineProperty(process.stdin, 'isTTY', {value: originalStdinIsTTY, configurable: true, writable: true}) - Object.defineProperty(process.stdout, 'isTTY', {value: originalStdoutIsTTY, configurable: true, writable: true}) + Object.defineProperty(process.stderr, 'isTTY', {value: originalStderrIsTTY, configurable: true, writable: true}) }) describe('Init command', () => { diff --git a/packages/cli-kit/src/private/node/ui/components/Prompts/PromptLayout.test.tsx b/packages/cli-kit/src/private/node/ui/components/Prompts/PromptLayout.test.tsx index 4b4d685af63..1b30dbdb3f1 100644 --- a/packages/cli-kit/src/private/node/ui/components/Prompts/PromptLayout.test.tsx +++ b/packages/cli-kit/src/private/node/ui/components/Prompts/PromptLayout.test.tsx @@ -1,11 +1,43 @@ import {PromptLayout} from './PromptLayout.js' import {PromptState} from '../../hooks/use-prompt.js' import {render} from '../../../testing/ui.js' -import {describe, expect, test} from 'vitest' +import {Stdout} from '../../../ui.js' +import {SelectInput} from '../SelectInput.js' +import {describe, expect, test, vi} from 'vitest' import React from 'react' import {Box, Text} from 'ink' describe('PromptLayout', async () => { + test('renders choices when the output stream has no terminal row count', async () => { + const stdout = new Stdout({columns: 80}) + Reflect.deleteProperty(stdout, 'rows') + const renderInstance = render( + {}} + /> + } + />, + {stdout}, + ) + + try { + await vi.waitFor(() => expect(stdout.lastFrame()).toContain('Second option')) + stdout.emit('resize') + await vi.waitFor(() => expect(stdout.lastFrame()).toContain('First option')) + } finally { + renderInstance.unmount() + renderInstance.cleanup() + } + }) + test("doesn't add unnecessary margins when infoTable is an empty array", async () => { const items = [ {label: 'first', value: 'first'}, diff --git a/packages/cli-kit/src/private/node/ui/components/Prompts/PromptLayout.tsx b/packages/cli-kit/src/private/node/ui/components/Prompts/PromptLayout.tsx index fe035922d80..422427c6367 100644 --- a/packages/cli-kit/src/private/node/ui/components/Prompts/PromptLayout.tsx +++ b/packages/cli-kit/src/private/node/ui/components/Prompts/PromptLayout.tsx @@ -11,6 +11,8 @@ import {Box, measureElement, Text, useStdout, DOMElement} from 'ink' import figures from 'figures' import type {InlineToken, LinkToken, TokenItem} from '../token-item.js' +const DEFAULT_TERMINAL_ROWS = 24 + export type Message = TokenItem> interface PromptLayoutProps { @@ -38,7 +40,7 @@ const PromptLayout = ({ const [wrapperHeight, setWrapperHeight] = useState(0) const [promptAreaHeight, setPromptAreaHeight] = useState(0) const [inputFixedAreaHeight, setInputFixedAreaHeight] = useState(0) - const currentAvailableLines = stdout.rows - promptAreaHeight - inputFixedAreaHeight + const currentAvailableLines = (stdout.rows ?? DEFAULT_TERMINAL_ROWS) - promptAreaHeight - inputFixedAreaHeight const [availableLines, setAvailableLines] = useState(currentAvailableLines) const wrapperRef = useCallback( @@ -72,7 +74,7 @@ const PromptLayout = ({ useLayoutEffect(() => { function onResize() { - const newAvailableLines = stdout.rows - promptAreaHeight - inputFixedAreaHeight + const newAvailableLines = (stdout.rows ?? DEFAULT_TERMINAL_ROWS) - promptAreaHeight - inputFixedAreaHeight if (newAvailableLines !== availableLines) { setAvailableLines(newAvailableLines) } diff --git a/packages/cli-kit/src/public/node/base-command.test.ts b/packages/cli-kit/src/public/node/base-command.test.ts index fb67fd5f482..21c0b43370b 100644 --- a/packages/cli-kit/src/public/node/base-command.test.ts +++ b/packages/cli-kit/src/public/node/base-command.test.ts @@ -14,21 +14,21 @@ import {Flags} from '@oclif/core' import {Ajv} from 'ajv' let originalStdinIsTTY: boolean | undefined -let originalStdoutIsTTY: boolean | undefined +let originalStderrIsTTY: boolean | undefined beforeEach(() => { originalStdinIsTTY = process.stdin.isTTY - originalStdoutIsTTY = process.stdout.isTTY + originalStderrIsTTY = process.stderr.isTTY vi.unstubAllEnvs() // Default: simulate interactive TTY environment Object.defineProperty(process.stdin, 'isTTY', {value: true, configurable: true, writable: true}) - Object.defineProperty(process.stdout, 'isTTY', {value: true, configurable: true, writable: true}) + Object.defineProperty(process.stderr, 'isTTY', {value: true, configurable: true, writable: true}) vi.stubEnv('CI', '') }) afterEach(() => { Object.defineProperty(process.stdin, 'isTTY', {value: originalStdinIsTTY, configurable: true, writable: true}) - Object.defineProperty(process.stdout, 'isTTY', {value: originalStdoutIsTTY, configurable: true, writable: true}) + Object.defineProperty(process.stderr, 'isTTY', {value: originalStderrIsTTY, configurable: true, writable: true}) mockAndCaptureOutput().clear() }) @@ -631,7 +631,7 @@ describe('applying environments', async () => { runTestInTmpDir('does not throw in TTY mode when a non-TTY required argument is missing', async (tmpDir: string) => { // Given — simulate interactive terminal Object.defineProperty(process.stdin, 'isTTY', {value: true, configurable: true, writable: true}) - Object.defineProperty(process.stdout, 'isTTY', {value: true, configurable: true, writable: true}) + Object.defineProperty(process.stderr, 'isTTY', {value: true, configurable: true, writable: true}) vi.stubEnv('CI', '') // When diff --git a/packages/cli-kit/src/public/node/system.ts b/packages/cli-kit/src/public/node/system.ts index 0659bc0b602..3d5659c418a 100644 --- a/packages/cli-kit/src/public/node/system.ts +++ b/packages/cli-kit/src/public/node/system.ts @@ -342,15 +342,15 @@ export function terminalSupportsHyperlinks(): boolean { } /** - * Check if the standard input and output streams support prompting. + * Check if standard input and standard error are terminals that support prompting. * - * @returns True if the standard input and output streams support prompting. + * @returns True if standard input and standard error support prompting. */ export function terminalSupportsPrompting(): boolean { if (isInputDisabled() || isTruthy(process.env.CI)) { return false } - return Boolean(process.stdin.isTTY && process.stdout.isTTY) + return Boolean(process.stdin.isTTY && process.stderr.isTTY) } /** diff --git a/packages/cli-kit/src/public/node/ui.test.ts b/packages/cli-kit/src/public/node/ui.test.ts index 1e8434936c5..83a968374e7 100644 --- a/packages/cli-kit/src/public/node/ui.test.ts +++ b/packages/cli-kit/src/public/node/ui.test.ts @@ -7,6 +7,10 @@ import { renderTasks, renderWarning, renderSingleTask, + renderSelectPrompt, + renderAutocompletePrompt, + renderTextPrompt, + renderDangerousConfirmationPrompt, Task, } from './ui.js' import {AbortSignal} from './abort.js' @@ -14,11 +18,12 @@ import {BugError, FatalError, AbortError, FatalErrorType} from './error.js' import {renderCommandEventAsJson, runWithCommandEvents, runWithCommandEventsForCommand} from './command-events.js' import {mockAndCaptureOutput, withCapturedStandardStreams} from './testing/output.js' import {outputResult, TokenizedString, unstyled} from './output.js' -import {Stdin} from '../../private/node/testing/ui.js' +import {Stdin, waitForInputsToBeReady} from '../../private/node/testing/ui.js' import {afterEach, beforeEach, describe, expect, test, vi} from 'vitest' import supportsHyperlinks from 'supports-hyperlinks' import {Writable} from 'stream' +import type {RenderOptions} from 'ink' vi.mock('supports-hyperlinks') @@ -30,6 +35,46 @@ afterEach(() => { mockAndCaptureOutput().clear() }) +test.each([ + { + name: 'select', + renderPrompt: (renderOptions: RenderOptions) => + renderSelectPrompt({message: 'Which option?', choices: [{label: 'First option', value: 'first'}], renderOptions}), + }, + { + name: 'autocomplete', + renderPrompt: (renderOptions: RenderOptions) => + renderAutocompletePrompt({ + message: 'Which option?', + choices: [{label: 'First option', value: 'first'}], + renderOptions, + }), + }, + { + name: 'text', + renderPrompt: (renderOptions: RenderOptions) => + renderTextPrompt({message: 'Which option?', defaultValue: 'first', renderOptions}), + }, + { + name: 'dangerous confirmation', + renderPrompt: (renderOptions: RenderOptions) => + renderDangerousConfirmationPrompt({message: 'Which option?', confirmation: 'first', renderOptions}), + input: '\u001b', + }, +])('renders $name prompts to stderr by default', async ({renderPrompt, input}) => { + const stdin = new Stdin() + + await withCapturedStandardStreams(async (streams) => { + const result = renderPrompt({stdin: stdin as unknown as NodeJS.ReadStream, debug: true, patchConsole: false}) + await waitForInputsToBeReady() + stdin.write(input ?? '\r') + await result + + expect(streams.stdout()).toBe('') + expect(streams.stderr()).toContain('Which option?') + }) +}) + describe('renderInfo', async () => { test('renders info inside a banner', async () => { // Given diff --git a/packages/cli-kit/src/public/node/ui.tsx b/packages/cli-kit/src/public/node/ui.tsx index ad2bda3c567..593433f0cfb 100644 --- a/packages/cli-kit/src/public/node/ui.tsx +++ b/packages/cli-kit/src/public/node/ui.tsx @@ -292,6 +292,7 @@ export async function renderSelectPrompt( }} />, { + stdout: process.stderr as unknown as NodeJS.WriteStream, ...renderOptions, exitOnCtrlC: false, }, @@ -444,6 +445,7 @@ export async function renderAutocompletePrompt( }} />, { + stdout: process.stderr as unknown as NodeJS.WriteStream, ...renderOptions, exitOnCtrlC: false, }, @@ -662,6 +664,7 @@ export async function renderTextPrompt( }} />, { + stdout: process.stderr as unknown as NodeJS.WriteStream, ...renderOptions, exitOnCtrlC: false, }, @@ -714,6 +717,7 @@ export async function renderDangerousConfirmationPrompt( }} />, { + stdout: process.stderr as unknown as NodeJS.WriteStream, ...renderOptions, exitOnCtrlC: false, }, diff --git a/packages/theme/src/cli/utilities/theme-ui.test.ts b/packages/theme/src/cli/utilities/theme-ui.test.ts index 9ef1d50018d..771d06e5345 100644 --- a/packages/theme/src/cli/utilities/theme-ui.test.ts +++ b/packages/theme/src/cli/utilities/theme-ui.test.ts @@ -10,6 +10,7 @@ vi.mock('@shopify/cli-kit/node/ui') beforeEach(() => vi.stubEnv('CI', '')) afterEach(() => { + vi.unstubAllGlobals() vi.unstubAllEnvs() }) @@ -42,7 +43,7 @@ describe('ensureDirectoryConfirmed', () => { vi.stubGlobal('process', { ...process, stdin: {...process.stdin, isTTY: true}, - stdout: {...process.stdout, isTTY: true}, + stderr: {...process.stderr, isTTY: true}, }) vi.mocked(renderConfirmationPrompt).mockResolvedValue(true) @@ -61,7 +62,7 @@ describe('ensureDirectoryConfirmed', () => { vi.stubGlobal('process', { ...process, stdin: {...process.stdin, isTTY: false}, - stdout: {...process.stdout, isTTY: true}, + stderr: {...process.stderr, isTTY: true}, }) const confirmed = await ensureDirectoryConfirmed(false) @@ -103,7 +104,7 @@ describe('ensureLiveThemeConfirmed', () => { vi.stubGlobal('process', { ...process, stdin: {...process.stdin, isTTY: true}, - stdout: {...process.stdout, isTTY: true}, + stderr: {...process.stderr, isTTY: true}, }) }) @@ -131,35 +132,41 @@ describe('ensureLiveThemeConfirmed', () => { expect(renderConfirmationPrompt).not.toHaveBeenCalled() }) - test('does not prompt for confirmation if acting on a live theme and allowLive flag is true', async () => { - // Given - await ensureLiveThemeConfirmed(liveTheme, 'start development mode', true) - - // Then - expect(renderConfirmationPrompt).not.toHaveBeenCalled() - }) - - test('requires --allow-live when input is disabled', async () => { + test('allows a live theme with --allow-live when prompting is unavailable', async () => { vi.stubEnv('SHOPIFY_FLAG_NO_INPUT', 'true') + vi.stubGlobal('process', { + ...process, + stdin: {...process.stdin, isTTY: false}, + stderr: {...process.stderr, isTTY: false}, + }) - const confirmation = ensureLiveThemeConfirmed(liveTheme, 'start development mode', false) + const result = await ensureLiveThemeConfirmed(liveTheme, 'start development mode', true) - await expect(confirmation).rejects.toThrow( - "Can't start development mode on the live theme when user input is unavailable.", - ) + expect(result).toBe(true) expect(renderConfirmationPrompt).not.toHaveBeenCalled() }) - test('preserves existing behavior for a live theme in a non-interactive environment', async () => { + test.each([ + {name: 'input is disabled', stdin: true, stderr: true, noInput: 'true'}, + {name: 'stdin is redirected', stdin: false, stderr: true}, + {name: 'stderr is redirected', stdin: true, stderr: false}, + {name: 'CI is enabled', stdin: true, stderr: true, ci: 'true'}, + ])('requires --allow-live when $name', async ({stdin, stderr, noInput, ci}) => { + vi.stubEnv('SHOPIFY_FLAG_NO_INPUT', noInput ?? '') + vi.stubEnv('CI', ci ?? '') vi.stubGlobal('process', { ...process, - stdin: {...process.stdin, isTTY: false}, + stdin: {...process.stdin, isTTY: stdin}, stdout: {...process.stdout, isTTY: true}, + stderr: {...process.stderr, isTTY: stderr}, }) - const result = await ensureLiveThemeConfirmed(liveTheme, 'start development mode', false) + const confirmation = ensureLiveThemeConfirmed(liveTheme, 'start development mode', false) - expect(result).toBe(true) + await expect(confirmation).rejects.toMatchObject({ + message: "Can't start development mode on the live theme when user input is unavailable.", + tryMessage: 'Use `--allow-live` to confirm that you want to continue.', + }) expect(renderConfirmationPrompt).not.toHaveBeenCalled() }) }) diff --git a/packages/theme/src/cli/utilities/theme-ui.ts b/packages/theme/src/cli/utilities/theme-ui.ts index e3f101bf28e..7362eb36e0a 100644 --- a/packages/theme/src/cli/utilities/theme-ui.ts +++ b/packages/theme/src/cli/utilities/theme-ui.ts @@ -65,15 +65,13 @@ export async function ensureLiveThemeConfirmed(theme: Theme, action: string, all return true } - if (isInputDisabled()) { + if (!terminalSupportsPrompting()) { throw new AbortError( `Can't ${action} on the live theme when user input is unavailable.`, 'Use `--allow-live` to confirm that you want to continue.', ) } - if (!terminalSupportsPrompting()) return true - const message = `You're about to ${action} on your live theme "${theme.name}". ` + `This will make changes visible to customers. Are you sure you want to proceed?`