From ae9721a5ba88447fd431fe163a3d8774f73c4e0c Mon Sep 17 00:00:00 2001 From: Elizabeth Craig Date: Mon, 18 Nov 2024 06:36:34 -0800 Subject: [PATCH] Move getQuestionsForPackage to its own file (#1006) --- ...-fbe57b6e-69d7-4628-8c28-6000d9e9449b.json | 7 ++ ...test.ts => getQuestionsForPackage.test.ts} | 32 +++--- .../changefile/promptForChange.test.ts | 21 ++-- ...ptForChange_promptForPackageChange.test.ts | 43 +++---- src/changefile/getQuestionsForPackage.ts | 107 ++++++++++++++++++ src/changefile/promptForChange.ts | 91 +-------------- 6 files changed, 166 insertions(+), 135 deletions(-) create mode 100644 change/beachball-fbe57b6e-69d7-4628-8c28-6000d9e9449b.json rename src/__tests__/changefile/{promptForChange_getQuestionsForPackage.test.ts => getQuestionsForPackage.test.ts} (86%) create mode 100644 src/changefile/getQuestionsForPackage.ts diff --git a/change/beachball-fbe57b6e-69d7-4628-8c28-6000d9e9449b.json b/change/beachball-fbe57b6e-69d7-4628-8c28-6000d9e9449b.json new file mode 100644 index 000000000..5a91d4c26 --- /dev/null +++ b/change/beachball-fbe57b6e-69d7-4628-8c28-6000d9e9449b.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "Internal refactor: getQuestionsForPackage to its own file", + "packageName": "beachball", + "email": "elcraig@microsoft.com", + "dependentChangeType": "patch" +} diff --git a/src/__tests__/changefile/promptForChange_getQuestionsForPackage.test.ts b/src/__tests__/changefile/getQuestionsForPackage.test.ts similarity index 86% rename from src/__tests__/changefile/promptForChange_getQuestionsForPackage.test.ts rename to src/__tests__/changefile/getQuestionsForPackage.test.ts index 9f3da1d5c..92ac8715d 100644 --- a/src/__tests__/changefile/promptForChange_getQuestionsForPackage.test.ts +++ b/src/__tests__/changefile/getQuestionsForPackage.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it, jest } from '@jest/globals'; import prompts from 'prompts'; -import { _getQuestionsForPackage } from '../../changefile/promptForChange'; +import { getQuestionsForPackage } from '../../changefile/getQuestionsForPackage'; import { ChangeFilePromptOptions } from '../../types/ChangeFilePrompt'; import { initMockLogs } from '../../__fixtures__/mockLogs'; import { makePackageInfos } from '../../__fixtures__/packageInfos'; @@ -8,12 +8,12 @@ import { makePackageInfos } from '../../__fixtures__/packageInfos'; /** * This covers the first part of `promptForChange`: determining what questions to ask for each package. */ -describe('promptForChange _getQuestionsForPackage', () => { +describe('getQuestionsForPackage', () => { /** Package name used in the tests */ const pkg = 'foo'; - /** Basic params for `_getQuestionsForPackage`, for a package named `foo` */ - const defaultQuestionsParams: Parameters[0] = { + /** Basic params for `getQuestionsForPackage`, for a package named `foo` */ + const defaultQuestionsParams: Parameters[0] = { pkg, packageInfos: makePackageInfos({ [pkg]: {} }), packageGroups: {}, @@ -24,7 +24,7 @@ describe('promptForChange _getQuestionsForPackage', () => { const logs = initMockLogs(); it('works in basic case', () => { - const questions = _getQuestionsForPackage(defaultQuestionsParams); + const questions = getQuestionsForPackage(defaultQuestionsParams); expect(questions).toEqual([ { choices: [ @@ -50,7 +50,7 @@ describe('promptForChange _getQuestionsForPackage', () => { // it's somewhat debatable if this is correct (maybe --type should be the override for disallowedChangeTypes?) it('errors if options.type is disallowed', () => { - const questions = _getQuestionsForPackage({ + const questions = getQuestionsForPackage({ ...defaultQuestionsParams, packageInfos: makePackageInfos({ [pkg]: { combinedOptions: { disallowedChangeTypes: ['major'] } } }), options: { type: 'major', message: '' }, @@ -60,7 +60,7 @@ describe('promptForChange _getQuestionsForPackage', () => { }); it('errors if there are no valid change types for package', () => { - const questions = _getQuestionsForPackage({ + const questions = getQuestionsForPackage({ ...defaultQuestionsParams, packageInfos: makePackageInfos({ [pkg]: { combinedOptions: { disallowedChangeTypes: ['major', 'minor', 'patch', 'none'] } }, @@ -71,7 +71,7 @@ describe('promptForChange _getQuestionsForPackage', () => { }); it('respects disallowedChangeTypes', () => { - const questions = _getQuestionsForPackage({ + const questions = getQuestionsForPackage({ ...defaultQuestionsParams, packageInfos: makePackageInfos({ [pkg]: { combinedOptions: { disallowedChangeTypes: ['major'] } } }), }); @@ -80,7 +80,7 @@ describe('promptForChange _getQuestionsForPackage', () => { }); it('allows prerelease change for package with prerelease version', () => { - const questions = _getQuestionsForPackage({ + const questions = getQuestionsForPackage({ ...defaultQuestionsParams, packageInfos: makePackageInfos({ [pkg]: { version: '1.0.0-beta.1' } }), }); @@ -90,7 +90,7 @@ describe('promptForChange _getQuestionsForPackage', () => { // this is a bit weird as well, but documenting current behavior it('excludes prerelease if disallowed', () => { - const questions = _getQuestionsForPackage({ + const questions = getQuestionsForPackage({ ...defaultQuestionsParams, packageInfos: makePackageInfos({ [pkg]: { version: '1.0.0-beta.1', combinedOptions: { disallowedChangeTypes: ['prerelease'] } }, @@ -101,7 +101,7 @@ describe('promptForChange _getQuestionsForPackage', () => { }); it('excludes the change type question when options.type is specified', () => { - const questions = _getQuestionsForPackage({ + const questions = getQuestionsForPackage({ ...defaultQuestionsParams, options: { type: 'patch', message: '' }, }); @@ -110,7 +110,7 @@ describe('promptForChange _getQuestionsForPackage', () => { }); it('excludes the change type question with only one valid option', () => { - const questions = _getQuestionsForPackage({ + const questions = getQuestionsForPackage({ ...defaultQuestionsParams, packageInfos: makePackageInfos({ [pkg]: { combinedOptions: { disallowedChangeTypes: ['major', 'minor', 'none'] } }, @@ -121,7 +121,7 @@ describe('promptForChange _getQuestionsForPackage', () => { }); it('excludes the change type question when prerelease is implicitly the only valid option', () => { - const questions = _getQuestionsForPackage({ + const questions = getQuestionsForPackage({ ...defaultQuestionsParams, packageInfos: makePackageInfos({ [pkg]: { @@ -135,7 +135,7 @@ describe('promptForChange _getQuestionsForPackage', () => { }); it('excludes the comment question when options.message is set', () => { - const questions = _getQuestionsForPackage({ + const questions = getQuestionsForPackage({ ...defaultQuestionsParams, options: { message: 'message' }, }); @@ -147,7 +147,7 @@ describe('promptForChange _getQuestionsForPackage', () => { const customQuestions: prompts.PromptObject[] = [{ name: 'custom', message: 'custom prompt', type: 'text' }]; const changePrompt: ChangeFilePromptOptions['changePrompt'] = jest.fn(() => customQuestions); - const questions = _getQuestionsForPackage({ + const questions = getQuestionsForPackage({ ...defaultQuestionsParams, packageInfos: makePackageInfos({ [pkg]: { combinedOptions: { changeFilePrompt: { changePrompt } } } }), }); @@ -166,7 +166,7 @@ describe('promptForChange _getQuestionsForPackage', () => { it('does case-insensitive filtering on description suggestions', async () => { const recentMessages = ['Foo', 'Bar', 'Baz']; const recentMessageChoices = [{ title: 'Foo' }, { title: 'Bar' }, { title: 'Baz' }]; - const questions = _getQuestionsForPackage({ ...defaultQuestionsParams, recentMessages }); + const questions = getQuestionsForPackage({ ...defaultQuestionsParams, recentMessages }); expect(questions).toEqual([ expect.anything(), expect.objectContaining({ diff --git a/src/__tests__/changefile/promptForChange.test.ts b/src/__tests__/changefile/promptForChange.test.ts index a0c8f0e24..a2e7b8a75 100644 --- a/src/__tests__/changefile/promptForChange.test.ts +++ b/src/__tests__/changefile/promptForChange.test.ts @@ -3,7 +3,6 @@ import prompts from 'prompts'; import { promptForChange, _getChangeFileInfoFromResponse, - _getQuestionsForPackage, _promptForPackageChange, } from '../../changefile/promptForChange'; import { ChangeFilePromptOptions } from '../../types/ChangeFilePrompt'; @@ -16,15 +15,17 @@ import { makePackageInfos } from '../../__fixtures__/packageInfos'; // so instead we inject a custom mock stdout stream, as well as stdin for entering answers let stdin: MockStdin; let stdout: MockStdout; -jest.mock( - 'prompts', - () => - ((questions, options) => { - questions = Array.isArray(questions) ? questions : [questions]; - questions = questions.map(q => ({ ...q, stdin, stdout })); - return (jest.requireActual('prompts') as typeof prompts)(questions, options); - }) as typeof prompts -); +jest.mock('prompts', () => { + const realPrompts = jest.requireActual('prompts') as typeof prompts; + + return ((questions, options) => { + const questionsArr = Array.isArray(questions) ? questions : [questions]; + return realPrompts( + questionsArr.map(q => ({ ...q, stdin, stdout })), + options + ); + }) as typeof prompts; +}); /** * These combined tests mainly cover various early bail-out cases (the only meaningful logic in diff --git a/src/__tests__/changefile/promptForChange_promptForPackageChange.test.ts b/src/__tests__/changefile/promptForChange_promptForPackageChange.test.ts index 23fa38e94..202df6d9d 100644 --- a/src/__tests__/changefile/promptForChange_promptForPackageChange.test.ts +++ b/src/__tests__/changefile/promptForChange_promptForPackageChange.test.ts @@ -1,24 +1,27 @@ import { afterEach, beforeEach, describe, expect, it, jest } from '@jest/globals'; import prompts from 'prompts'; -import { _getQuestionsForPackage, _promptForPackageChange } from '../../changefile/promptForChange'; +import { _promptForPackageChange } from '../../changefile/promptForChange'; import { initMockLogs } from '../../__fixtures__/mockLogs'; import { MockStdin } from '../../__fixtures__/mockStdin'; import { MockStdout } from '../../__fixtures__/mockStdout'; import { makePackageInfos } from '../../__fixtures__/packageInfos'; +import { getQuestionsForPackage } from '../../changefile/getQuestionsForPackage'; // prompts writes to stdout (not console) in a way that can't really be mocked with spies, // so instead we inject a custom mock stdout stream, as well as stdin for entering answers let stdin: MockStdin; let stdout: MockStdout; -jest.mock( - 'prompts', - () => - ((questions, options) => { - questions = Array.isArray(questions) ? questions : [questions]; - questions = questions.map(q => ({ ...q, stdin, stdout })); - return (jest.requireActual('prompts') as typeof prompts)(questions, options); - }) as typeof prompts -); +jest.mock('prompts', () => { + const realPrompts = jest.requireActual('prompts') as typeof prompts; + + return ((questions, options) => { + const questionsArr = Array.isArray(questions) ? questions : [questions]; + return realPrompts( + questionsArr.map(q => ({ ...q, stdin, stdout })), + options + ); + }) as typeof prompts; +}); /** * This covers the actual prompting part of `promptForChange`. @@ -27,8 +30,8 @@ describe('promptForChange _promptForPackageChange', () => { /** Package name used in the tests */ const pkg = 'foo'; - /** Basic params for `_getQuestionsForPackage`, for a package named `foo` */ - const defaultQuestionsParams: Parameters[0] = { + /** Basic params for `getQuestionsForPackage`, for a package named `foo` */ + const defaultQuestionsParams: Parameters[0] = { pkg, packageInfos: makePackageInfos({ [pkg]: {} }), packageGroups: {}, @@ -64,7 +67,7 @@ describe('promptForChange _promptForPackageChange', () => { }); it('prompts for change type and description', async () => { - const questions = _getQuestionsForPackage(defaultQuestionsParams); + const questions = getQuestionsForPackage(defaultQuestionsParams); expect(questions).toEqual(expectedQuestions); const answersPromise = _promptForPackageChange(questions!, pkg); @@ -90,7 +93,7 @@ describe('promptForChange _promptForPackageChange', () => { it('accepts custom description typed by character', async () => { // For this one we provide a type in options and only ask for the description - const questions = _getQuestionsForPackage({ + const questions = getQuestionsForPackage({ ...defaultQuestionsParams, options: { type: 'minor', message: '' }, }); @@ -121,7 +124,7 @@ describe('promptForChange _promptForPackageChange', () => { it('accepts custom description pasted', async () => { // For this one we provide a type in options and only ask for the description - const questions = _getQuestionsForPackage({ + const questions = getQuestionsForPackage({ ...defaultQuestionsParams, options: { type: 'minor', message: '' }, }); @@ -151,7 +154,7 @@ describe('promptForChange _promptForPackageChange', () => { it('accepts custom description pasted with newline', async () => { // For this one we provide a type in options and only ask for the description - const questions = _getQuestionsForPackage({ + const questions = getQuestionsForPackage({ ...defaultQuestionsParams, options: { type: 'minor', message: '' }, }); @@ -177,7 +180,7 @@ describe('promptForChange _promptForPackageChange', () => { it('uses options selected with arrow keys', async () => { const recentMessages = ['first', 'second', 'third']; - const questions = _getQuestionsForPackage({ ...defaultQuestionsParams, recentMessages }); + const questions = getQuestionsForPackage({ ...defaultQuestionsParams, recentMessages }); expect(questions).toEqual(expectedQuestions); const answerPromise = _promptForPackageChange(questions!, pkg); @@ -223,7 +226,7 @@ describe('promptForChange _promptForPackageChange', () => { }); it('filters options while typing', async () => { - const questions = _getQuestionsForPackage({ + const questions = getQuestionsForPackage({ ...defaultQuestionsParams, recentMessages: ['foo', 'bar', 'baz'], options: { type: 'minor', message: '' }, @@ -254,7 +257,7 @@ describe('promptForChange _promptForPackageChange', () => { }); it('handles pressing delete while typing', async () => { - const questions = _getQuestionsForPackage({ + const questions = getQuestionsForPackage({ ...defaultQuestionsParams, recentMessages: ['foo', 'bar', 'baz'], options: { type: 'minor', message: '' }, @@ -288,7 +291,7 @@ describe('promptForChange _promptForPackageChange', () => { }); it('returns no answers if cancelled with ctrl-c', async () => { - const questions = _getQuestionsForPackage(defaultQuestionsParams); + const questions = getQuestionsForPackage(defaultQuestionsParams); expect(questions).toEqual(expectedQuestions); const answerPromise = _promptForPackageChange(questions!, pkg); diff --git a/src/changefile/getQuestionsForPackage.ts b/src/changefile/getQuestionsForPackage.ts new file mode 100644 index 000000000..3afa20398 --- /dev/null +++ b/src/changefile/getQuestionsForPackage.ts @@ -0,0 +1,107 @@ +import prompts from 'prompts'; +import semver from 'semver'; +import { ChangeType } from '../types/ChangeInfo'; +import { BeachballOptions } from '../types/BeachballOptions'; +import { DefaultPrompt } from '../types/ChangeFilePrompt'; +import { getDisallowedChangeTypes } from './getDisallowedChangeTypes'; +import { PackageGroups, PackageInfos } from '../types/PackageInfo'; + +/** + * Build the list of questions to ask the user for this package. + * Also validates the options and returns undefined if there's an issue. + */ +export function getQuestionsForPackage(params: { + pkg: string; + packageInfos: PackageInfos; + packageGroups: PackageGroups; + options: Pick; + recentMessages: string[]; +}): prompts.PromptObject[] | undefined { + const { pkg, packageInfos, options, recentMessages } = params; + const packageInfo = packageInfos[pkg]; + + const changeTypePrompt = getChangeTypePrompt(params); + if (!changeTypePrompt) { + return; + } + + const defaultPrompt: DefaultPrompt = { + changeType: !options.type && changeTypePrompt.choices!.length > 1 ? changeTypePrompt : undefined, + description: !options.message ? getDescriptionPrompt(recentMessages) : undefined, + }; + + const questions = + packageInfo.combinedOptions.changeFilePrompt?.changePrompt?.(defaultPrompt, pkg) || Object.values(defaultPrompt); + + return questions.filter((q): q is prompts.PromptObject => !!q); +} + +function getChangeTypePrompt(params: { + pkg: string; + packageInfos: PackageInfos; + packageGroups: PackageGroups; + options: Pick; +}): prompts.PromptObject | undefined { + const { pkg, packageInfos, packageGroups, options } = params; + const packageInfo = packageInfos[pkg]; + + const disallowedChangeTypes = getDisallowedChangeTypes(pkg, packageInfos, packageGroups) || []; + + if (options.type && disallowedChangeTypes.includes(options.type as ChangeType)) { + console.error(`Change type "${options.type}" is not allowed for package "${pkg}"`); + return; + } + + const showPrereleaseOption = !!semver.prerelease(packageInfo.version); + const changeTypeChoices: prompts.Choice[] = [ + ...(showPrereleaseOption ? [{ value: 'prerelease', title: ' Prerelease - bump prerelease version' }] : []), + { value: 'patch', title: ' Patch - bug fixes; no API changes.' }, + { value: 'minor', title: ' Minor - small feature; backwards compatible API changes.' }, + { + value: 'none', + title: ' None - this change does not affect the published package in any way.', + }, + { value: 'major', title: ' Major - major feature; breaking changes.' }, + ].filter(choice => !disallowedChangeTypes?.includes(choice.value as ChangeType)); + + if (!changeTypeChoices.length) { + console.error(`No valid change types available for package "${pkg}"`); + return; + } + + return { + type: 'select', + name: 'type', + message: 'Change type', + choices: changeTypeChoices, + }; +} + +function getDescriptionPrompt(recentMessages: string[]): prompts.PromptObject { + // Do case-insensitive filtering of recent commit messages + const recentMessageChoices: prompts.Choice[] = recentMessages.map(msg => ({ title: msg })); + const getSuggestions = (input: string) => + input + ? recentMessageChoices.filter(({ title }) => title.toLowerCase().startsWith(input.toLowerCase())) + : recentMessageChoices; + + return { + type: 'autocomplete', + name: 'comment', + message: 'Describe changes (type or choose one)', + choices: recentMessageChoices, + suggest: (input: string) => Promise.resolve(getSuggestions(input)), + // prompts doesn't have proper support for "freeform" input (value not in the list), and the + // previously implemented hack of adding the input to the returned list from `suggest` + // no longer works. So this new hack adds the current input as the fallback. + // https://github.com/terkelg/prompts/issues/131 + onState: function (this: any, state: any) { + // If there are no suggestions, update the value to match the input, and unset the fallback + // (this.suggestions may be out of date if the user pasted text ending with a newline, so re-calculate) + if (!getSuggestions(this.input).length) { + this.value = this.input; + this.fallback = ''; + } + }, + }; +} diff --git a/src/changefile/promptForChange.ts b/src/changefile/promptForChange.ts index fbdfbc759..13815fd64 100644 --- a/src/changefile/promptForChange.ts +++ b/src/changefile/promptForChange.ts @@ -1,11 +1,9 @@ import prompts from 'prompts'; -import { prerelease } from 'semver'; import { ChangeFileInfo, ChangeType } from '../types/ChangeInfo'; import { BeachballOptions } from '../types/BeachballOptions'; import { isValidChangeType } from '../validation/isValidChangeType'; -import { DefaultPrompt } from '../types/ChangeFilePrompt'; -import { getDisallowedChangeTypes } from './getDisallowedChangeTypes'; import { PackageGroups, PackageInfos } from '../types/PackageInfo'; +import { getQuestionsForPackage } from './getQuestionsForPackage'; type ChangePromptResponse = { type?: ChangeType; comment?: string }; @@ -29,7 +27,7 @@ export async function promptForChange(params: { // Get the questions for each package first, in case one package has a validation issue const packageQuestions: { [pkg: string]: prompts.PromptObject[] } = {}; for (const pkg of changedPackages) { - const questions = _getQuestionsForPackage({ pkg, ...params }); + const questions = getQuestionsForPackage({ pkg, ...params }); if (!questions) { return; // validation issue } @@ -54,91 +52,6 @@ export async function promptForChange(params: { return packageChangeInfo; } -/** - * Build the list of questions to ask the user for this package. - * @internal exported for testing - */ -export function _getQuestionsForPackage(params: { - pkg: string; - packageInfos: PackageInfos; - packageGroups: PackageGroups; - options: Pick; - recentMessages: string[]; -}): prompts.PromptObject[] | undefined { - const { pkg, packageInfos, packageGroups, options, recentMessages } = params; - - const disallowedChangeTypes = getDisallowedChangeTypes(pkg, packageInfos, packageGroups); - - if (options.type && disallowedChangeTypes?.includes(options.type as ChangeType)) { - console.error(`Change type "${options.type}" is not allowed for package "${pkg}"`); - return; - } - - const packageInfo = packageInfos[pkg]; - const showPrereleaseOption = !!prerelease(packageInfo.version); - const changeTypeChoices: prompts.Choice[] = [ - ...(showPrereleaseOption ? [{ value: 'prerelease', title: ' Prerelease - bump prerelease version' }] : []), - { value: 'patch', title: ' Patch - bug fixes; no API changes.' }, - { value: 'minor', title: ' Minor - small feature; backwards compatible API changes.' }, - { - value: 'none', - title: ' None - this change does not affect the published package in any way.', - }, - { value: 'major', title: ' Major - major feature; breaking changes.' }, - ].filter(choice => !disallowedChangeTypes?.includes(choice.value as ChangeType)); - - if (!changeTypeChoices.length) { - console.error(`No valid change types available for package "${pkg}"`); - return; - } - - const changeTypePrompt: prompts.PromptObject = { - type: 'select', - name: 'type', - message: 'Change type', - choices: changeTypeChoices, - }; - - // Do case-insensitive filtering of recent commit messages - const recentMessageChoices: prompts.Choice[] = recentMessages.map(msg => ({ title: msg })); - const getSuggestions = (input: string) => - input - ? recentMessageChoices.filter(({ title }) => title.toLowerCase().startsWith(input.toLowerCase())) - : recentMessageChoices; - - const descriptionPrompt: prompts.PromptObject = { - type: 'autocomplete', - name: 'comment', - message: 'Describe changes (type or choose one)', - choices: recentMessageChoices, - suggest: (input: string) => Promise.resolve(getSuggestions(input)), - // prompts doesn't have proper support for "freeform" input (value not in the list), and the - // previously implemented hack of adding the input to the returned list from `suggest` - // no longer works. So this new hack adds the current input as the fallback. - // https://github.com/terkelg/prompts/issues/131 - onState: function (this: any, state: any) { - // If there are no suggestions, update the value to match the input, and unset the fallback - // (this.suggestions may be out of date if the user pasted text ending with a newline, so re-calculate) - if (!getSuggestions(this.input).length) { - this.value = this.input; - this.fallback = ''; - } - }, - }; - - const showChangeTypePrompt = !options.type && changeTypePrompt.choices!.length > 1; - - const defaultPrompt: DefaultPrompt = { - changeType: showChangeTypePrompt ? changeTypePrompt : undefined, - description: !options.message ? descriptionPrompt : undefined, - }; - const defaultPrompts = [defaultPrompt.changeType, defaultPrompt.description]; - - return (packageInfo.combinedOptions.changeFilePrompt?.changePrompt?.(defaultPrompt, pkg) || defaultPrompts).filter( - (q): q is prompts.PromptObject => !!q - ); -} - /** * Do the actual prompting. * @internal exported for testing