From 9734b9f9bce5ededc92c52c4eb14663a9e998893 Mon Sep 17 00:00:00 2001 From: mshanemc Date: Fri, 7 Jul 2023 14:08:14 -0500 Subject: [PATCH 1/5] feat: perf benchmarks --- package.json | 7 ++- perf.txt | 6 +++ test/perf/parser.perf.ts | 99 ++++++++++++++++++++++++++++++++++++++++ yarn.lock | 20 +++++++- 4 files changed, 129 insertions(+), 3 deletions(-) create mode 100644 perf.txt create mode 100644 test/perf/parser.perf.ts diff --git a/package.json b/package.json index 259c2697e..a4f408673 100644 --- a/package.json +++ b/package.json @@ -41,6 +41,7 @@ "@oclif/plugin-plugins": "^2.4.7", "@oclif/test": "^2.3.15", "@types/ansi-styles": "^3.2.1", + "@types/benchmark": "^2.1.2", "@types/chai": "^4.3.4", "@types/chai-as-promised": "^7.1.5", "@types/clean-stack": "^2.1.1", @@ -59,6 +60,7 @@ "@types/supports-color": "^8.1.1", "@types/wordwrap": "^1.0.1", "@types/wrap-ansi": "^3.0.0", + "benchmark": "^2.1.4", "chai": "^4.3.7", "chai-as-promised": "^7.1.1", "commitlint": "^12.1.4", @@ -111,7 +113,8 @@ "prepack": "yarn run build", "test": "mocha --forbid-only \"test/**/*.test.ts\"", "test:e2e": "mocha --forbid-only \"test/**/*.e2e.ts\" --timeout 1200000", - "pretest": "yarn build --noEmit && tsc -p test --noEmit --skipLibCheck" + "pretest": "yarn build --noEmit && tsc -p test --noEmit --skipLibCheck", + "test:perf": "ts-node test/perf/parser.perf.ts" }, "types": "lib/index.d.ts" -} \ No newline at end of file +} diff --git a/perf.txt b/perf.txt new file mode 100644 index 000000000..b6928c781 --- /dev/null +++ b/perf.txt @@ -0,0 +1,6 @@ +yarn run v1.22.19 +$ ts-node test/perf/parser.perf.ts +simple x 103,392 ops/sec ±0.91% (77 runs sampled) +multiple async flags that take time x 4.96 ops/sec ±0.13% (28 runs sampled) +multiple async flags that take time x 5,451 ops/sec ±2.09% (79 runs sampled) +Done in 21.11s. diff --git a/test/perf/parser.perf.ts b/test/perf/parser.perf.ts new file mode 100644 index 000000000..da529f3c1 --- /dev/null +++ b/test/perf/parser.perf.ts @@ -0,0 +1,99 @@ +/* + * Copyright (c) 2023, salesforce.com, inc. + * All rights reserved. + * Licensed under the BSD 3-Clause license. + * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause + */ +import {Suite} from 'benchmark' +import {parse} from '../../src/parser' +import {Flags} from '../../src' + +const suite = new Suite() + +// eslint-disable-next-line no-promise-executor-return +const delay100 = () => new Promise(resolve => setTimeout(resolve, 100)) + +suite +.add('simple', + { + defer: true, + fn: function (deferred: { resolve: () => any }) { + parse(['--bool'], { + flags: { + bool: Flags.boolean(), + }, + }).then(() => deferred.resolve()) + }, + }) +.add('multiple async flags that take time', { + defer: true, + fn: function (deferred: { resolve: () => any }) { + parse(['--flagA', 'foo', '--flagA', 'bar'], { + flags: { + flagA: Flags.string({ + parse: async input => { + await delay100() + return input + }, + }), + flagB: Flags.string({ + parse: async input => { + await delay100() + return input + }, + }), + }, + }).then(() => deferred.resolve()) + }, +}) + +.add('flagstravaganza', { + defer: true, + fn: function (deferred: { resolve: () => any }) { + const flags = [ + ['--bool'], + ['-S', 'foo'], + + ['--dep-string', 'foo'], + ['--excl-string', 'foo'], + ['--exactly-one', 'foo'], + + ['--parsed-string-as-number', '5'], + ['--dir', process.cwd()], + ['--file', __filename], + + ['--multiple', '1'], + ['--multiple', '2'], + ['--multiple', '3'], + ['--multiple2', '5,6,7,8,9,10,11'], + ] + const exactlyOne = ['exactly-one-nope', 'exactly-one-nope2', 'exactly-one'] + parse(flags.flat(), { + flags: { + bool: Flags.boolean(), + string: Flags.string({char: 's', aliases: ['S']}), + + 'dep-string': Flags.string({dependsOn: ['string']}), + // don't populate this one, used for exclusive test + nope: Flags.boolean(), + 'excl-string': Flags.string({exclusive: ['nope']}), + 'exactly-one-nope': Flags.string({exactlyOne}), + 'exactly-one-nope2': Flags.string({exactlyOne}), + 'exactly-one': Flags.string({exactlyOne}), + + 'parsed-string-as-number': Flags.integer({parse: input => Promise.resolve(Number.parseInt(input, 10) + 1000)}), + dir: Flags.directory({exists: true}), + file: Flags.file({exists: true}), + + multiple: Flags.string({multiple: true}), + multiple2: Flags.string({multiple: true, delimiter: ','}), + }, + }).then(() => deferred.resolve()) + }, +}) + +// add listeners +.on('cycle', (event: any) => { + console.log(String(event.target)) +}) +.run({async: true}) diff --git a/yarn.lock b/yarn.lock index 3cdead2f3..7d74195ae 100644 --- a/yarn.lock +++ b/yarn.lock @@ -567,6 +567,11 @@ dependencies: "@types/color-name" "*" +"@types/benchmark@^2.1.2": + version "2.1.2" + resolved "https://registry.yarnpkg.com/@types/benchmark/-/benchmark-2.1.2.tgz#b7838408c93dc08ceb4e6e13147dbfbe6a151f82" + integrity sha512-EDKtLYNMKrig22jEvhXq8TBFyFgVNSPmDF2b9UzJ7+eylPqdZVo17PCUMkn1jP6/1A/0u78VqYC6VrX6b8pDWA== + "@types/chai-as-promised@^7.1.5": version "7.1.5" resolved "https://registry.yarnpkg.com/@types/chai-as-promised/-/chai-as-promised-7.1.5.tgz#6e016811f6c7a64f2eed823191c3a6955094e255" @@ -982,6 +987,14 @@ balanced-match@^1.0.0: resolved "https://registry.yarnpkg.com/balanced-match/-/balanced-match-1.0.2.tgz#e83e3a7e3f300b34cb9d87f615fa0cbf357690ee" integrity sha512-3oSeUO0TMV67hN1AmbXsK4yaqU7tjiHlbxRDZOpH0KW9+CeX4bRAaX0Anxt0tx2MrpRpWwQaPwIlISEJhYU5Pw== +benchmark@^2.1.4: + version "2.1.4" + resolved "https://registry.yarnpkg.com/benchmark/-/benchmark-2.1.4.tgz#09f3de31c916425d498cc2ee565a0ebf3c2a5629" + integrity sha512-l9MlfN4M1K/H2fbhfMy3B7vJd6AGKJVQn2h6Sg/Yx+KckoUA7ewS5Vv6TjSq18ooE1kS9hhAlQRH3AkXIh/aOQ== + dependencies: + lodash "^4.17.4" + platform "^1.3.3" + binary-extensions@^2.0.0: version "2.2.0" resolved "https://registry.yarnpkg.com/binary-extensions/-/binary-extensions-2.2.0.tgz#75f502eeaf9ffde42fc98829645be4ea76bd9e2d" @@ -2300,7 +2313,7 @@ lodash.truncate@^4.4.2: resolved "https://registry.yarnpkg.com/lodash.truncate/-/lodash.truncate-4.4.2.tgz#5a350da0b1113b837ecfffd5812cbe58d6eae193" integrity sha1-WjUNoLERO4N+z//VgSy+WNbq4ZM= -lodash@^4.17.13, lodash@^4.17.15, lodash@^4.17.19, lodash@^4.17.21: +lodash@^4.17.13, lodash@^4.17.15, lodash@^4.17.19, lodash@^4.17.21, lodash@^4.17.4: version "4.17.21" resolved "https://registry.yarnpkg.com/lodash/-/lodash-4.17.21.tgz#679591c564c3bffaae8454cf0b3df370c3d6911c" integrity sha512-v2kDEe57lecTulaDIuNTPy3Ry4gLGJ6Z1O3vE1krgXZNrsQ+LFTGHVxVjcXPs17LhbZVGedAJv8XZ1tvj5FvSg== @@ -2709,6 +2722,11 @@ pify@^4.0.1: resolved "https://registry.yarnpkg.com/pify/-/pify-4.0.1.tgz#4b2cd25c50d598735c50292224fd8c6df41e3231" integrity sha512-uB80kBFb/tfd68bVleG9T5GGsGPjJrLAUpR5PZIrhBnIaRTQRjqdJSsIKkOP6OAIFbj7GOrcudc5pNjZ+geV2g== +platform@^1.3.3: + version "1.3.6" + resolved "https://registry.yarnpkg.com/platform/-/platform-1.3.6.tgz#48b4ce983164b209c2d45a107adb31f473a6e7a7" + integrity sha512-fnWVljUchTro6RiCFvCXBbNhJc2NijN7oIQxbwsyL0buWJPG85v81ehlHI9fXrJsMNgTofEoWIQeClKpgxFLrg== + plur@^4.0.0: version "4.0.0" resolved "https://registry.yarnpkg.com/plur/-/plur-4.0.0.tgz#729aedb08f452645fe8c58ef115bf16b0a73ef84" From a7f3e5faadd300752a4ff4ee334d010159a65fdb Mon Sep 17 00:00:00 2001 From: mshanemc Date: Fri, 7 Jul 2023 14:59:33 -0500 Subject: [PATCH 2/5] chore: comments --- src/parser/parse.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/parser/parse.ts b/src/parser/parse.ts index ba6ceab35..64eb6f7a0 100644 --- a/src/parser/parse.ts +++ b/src/parser/parse.ts @@ -213,6 +213,8 @@ export class Parser { const flags = {} as any this.metaData.flags = {} as any + + // parse the flags that the user provided for (const token of this._flagTokens) { const flag = this.input.flags[token.flag] @@ -256,7 +258,9 @@ export class Parser Date: Fri, 7 Jul 2023 14:59:53 -0500 Subject: [PATCH 3/5] refactor: remove redundant vars --- src/parser/parse.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/parser/parse.ts b/src/parser/parse.ts index 64eb6f7a0..64e11558a 100644 --- a/src/parser/parse.ts +++ b/src/parser/parse.ts @@ -310,8 +310,6 @@ export class Parser Date: Mon, 10 Jul 2023 11:31:17 -0500 Subject: [PATCH 4/5] refactor: parallelization of async flag parsing --- perf.txt | 8 +- src/interfaces/parser.ts | 2 +- src/parser/parse.ts | 241 ++++++++++++++++++++++----------------- src/parser/validate.ts | 71 ++++++------ 4 files changed, 177 insertions(+), 145 deletions(-) diff --git a/perf.txt b/perf.txt index b6928c781..5c7d34bcc 100644 --- a/perf.txt +++ b/perf.txt @@ -1,6 +1,6 @@ yarn run v1.22.19 $ ts-node test/perf/parser.perf.ts -simple x 103,392 ops/sec ±0.91% (77 runs sampled) -multiple async flags that take time x 4.96 ops/sec ±0.13% (28 runs sampled) -multiple async flags that take time x 5,451 ops/sec ±2.09% (79 runs sampled) -Done in 21.11s. +simple x 131,268 ops/sec ±1.15% (79 runs sampled) +multiple async flags that take time x 9.90 ops/sec ±0.14% (50 runs sampled) +flagstravaganza x 7,425 ops/sec ±2.10% (83 runs sampled) +Done in 21.60s. diff --git a/src/interfaces/parser.ts b/src/interfaces/parser.ts index 5b2f15fb6..58abb381b 100644 --- a/src/interfaces/parser.ts +++ b/src/interfaces/parser.ts @@ -41,7 +41,7 @@ export type Metadata = { flags: { [key: string]: MetadataFlag }; } -type MetadataFlag = { +export type MetadataFlag = { setFromDefault?: boolean; defaultHelp?: unknown; } diff --git a/src/parser/parse.ts b/src/parser/parse.ts index 64e11558a..169d0450f 100644 --- a/src/parser/parse.ts +++ b/src/parser/parse.ts @@ -1,6 +1,6 @@ /* eslint-disable no-await-in-loop */ import {ArgInvalidOptionError, CLIError, FlagInvalidOptionError} from './errors' -import {ArgToken, BooleanFlag, FlagToken, OptionFlag, OutputArgs, OutputFlags, ParserInput, ParserOutput, ParsingToken} from '../interfaces/parser' +import {ArgToken, BooleanFlag, CompletableFlag, FlagToken, Metadata, MetadataFlag, OptionFlag, OutputArgs, OutputFlags, ParserInput, ParserOutput, ParsingToken} from '../interfaces/parser' import * as readline from 'readline' import {isTruthy, pickBy} from '../util' @@ -67,8 +67,6 @@ export class Parser constructor(private readonly input: T) { @@ -79,8 +77,6 @@ export class Parser { return (flag.aliases ?? []).map(a => [a, flag]) })) - - this.metaData = {} } public async parse(): Promise> { @@ -193,8 +189,7 @@ export class Parser { - const flags = {} as any - this.metaData.flags = {} as any + private async _flags(): Promise<{ + flags: TFlags & BFlags & { json: boolean | undefined }, metadata: Metadata + }> { + type ValueFunction = (fws: FlagWithStrategy, flags?: Record) => Promise - // parse the flags that the user provided - for (const token of this._flagTokens) { - const flag = this.input.flags[token.flag] + const validateOptions = (flag: OptionFlag, input: string): string => { + if (flag.options && !flag.options.includes(input)) + throw new FlagInvalidOptionError(flag, input) + return input + } - if (!flag) throw new CLIError(`Unexpected flag ${token.flag}`) + const parseFlagOrThrowError = async (input: any, flag: BooleanFlag | OptionFlag, token?: FlagToken, context: typeof this.context = {}) => { + if (!flag.parse) return input - if (flag.type === 'boolean') { - if (token.input === `--no-${flag.name}`) { - flags[token.flag] = false - } else { - flags[token.flag] = true + try { + if (flag.type === 'boolean') { + return await flag.parse(input, {...context, token}, flag) } - flags[token.flag] = await this._parseFlag(flags[token.flag], flag, token) - } else { - const input = token.input - - if (flag.delimiter && flag.multiple) { - // split, trim, and remove surrounding doubleQuotes (which would hav been needed if the elements contain spaces) - const values = await Promise.all( - input.split(flag.delimiter).map(async v => this._parseFlag(v.trim().replace(/^"(.*)"$/, '$1').replace(/^'(.*)'$/, '$1'), flag, token)), - ) - // then parse that each element aligns with the `options` property - for (const v of values) { - this._validateOptions(flag, v) - } + return await flag.parse(input, {...context, token}, flag) + } catch (error: any) { + error.message = `Parsing --${flag.name} \n\t${error.message}\nSee more help with --help` + throw error + } + } - flags[token.flag] = flags[token.flag] || [] - flags[token.flag].push(...values) - } else { - this._validateOptions(flag, input) - const value = await this._parseFlag(input, flag, token) - if (flag.multiple) { - flags[token.flag] = flags[token.flag] || [] - flags[token.flag].push(value) - } else { - flags[token.flag] = value + /* Could add a valueFunction (if there is a value/env/default) and could metadata. + * Value function can be resolved later. + */ + const addValueFunction = (fws: FlagWithStrategy): FlagWithStrategy => { + const tokenLength = fws.tokens?.length + // user provided some input + if (tokenLength) { + // boolean + if (fws.inputFlag.flag.type === 'boolean' && fws.tokens?.at(-1)?.input) { + return {...fws, valueFunction: async (i: FlagWithStrategy) => parseFlagOrThrowError(i.tokens?.at(-1)?.input !== `--no-${i.inputFlag.name}`, i.inputFlag.flag, i.tokens?.at(-1), this.context)} + } + + // multiple with custom delimiter + if (fws.inputFlag.flag.type === 'option' && fws.inputFlag.flag.delimiter && fws.inputFlag.flag.multiple) { + return { + ...fws, valueFunction: async (i: FlagWithStrategy) => (await Promise.all( + ((i.tokens ?? []).flatMap(token => (token.input as string).split(i.inputFlag.flag.delimiter as string))) + // trim, and remove surrounding doubleQuotes (which would hav been needed if the elements contain spaces) + .map(v => v.trim().replace(/^"(.*)"$/, '$1').replace(/^'(.*)'$/, '$1')) + .map(async v => parseFlagOrThrowError(v, i.inputFlag.flag, {...i.tokens?.at(-1) as FlagToken, input: v}, this.context)), + )).map(v => validateOptions(i.inputFlag.flag as OptionFlag, v)), } } + + // multiple in the oclif-core style + if (fws.inputFlag.flag.type === 'option' && fws.inputFlag.flag.multiple) { + return {...fws, valueFunction: async (i: FlagWithStrategy) => Promise.all((fws.tokens ?? []).map(token => parseFlagOrThrowError(validateOptions(i.inputFlag.flag as OptionFlag, token.input as string), i.inputFlag.flag, token, this.context)))} + } + + // simple option flag + if (fws.inputFlag.flag.type === 'option') { + return {...fws, valueFunction: async (i: FlagWithStrategy) => parseFlagOrThrowError(validateOptions(i.inputFlag.flag as OptionFlag, fws.tokens?.at(-1)?.input as string), i.inputFlag.flag, fws.tokens?.at(-1), this.context)} + } } - } - for (const k of Object.keys(this.input.flags)) { - const flag = this.input.flags[k] - // we already did this one from flagTokens - if (flags[k]) continue - // env flags might not have a token,but get their value from the environment, so we parse those - if (flag.env && Reflect.has(process.env, flag.env)) { - const input = process.env[flag.env] - if (flag.type === 'option') { - if (input) { - this._validateOptions(flag, input) - - flags[k] = await this._parseFlag(input, flag) - } - } else if (flag.type === 'boolean') { - // eslint-disable-next-line no-negated-condition - flags[k] = input !== undefined ? isTruthy(input) : false + // no input: env flags + if (fws.inputFlag.flag.env && process.env[fws.inputFlag.flag.env]) { + const valueFromEnv = process.env[fws.inputFlag.flag.env] + if (fws.inputFlag.flag.type === 'option' && typeof valueFromEnv === 'string') { + return {...fws, valueFunction: async (i: FlagWithStrategy) => parseFlagOrThrowError(validateOptions(i.inputFlag.flag as OptionFlag, valueFromEnv), i.inputFlag.flag, this.context)} + } + + if (fws.inputFlag.flag.type === 'boolean') { + return {...fws, valueFunction: async (i: FlagWithStrategy) => Promise.resolve(isTruthy(process.env[i.inputFlag.flag.env as string] ?? 'false'))} } } - // flags with a default don't have a token, but we want to evaluate their default - if (!(k in flags) && flag.default !== undefined) { - this.metaData.flags[k] = {...this.metaData.flags[k], setFromDefault: true} - const defaultValue = (typeof flag.default === 'function' ? await flag.default({options: flag, flags}) : flag.default) - flags[k] = defaultValue + // no input, but flag has default value + if (typeof fws.inputFlag.flag.default !== undefined) { + return { + ...fws, metadata: {setFromDefault: true}, + valueFunction: typeof fws.inputFlag.flag.default === 'function' ? + (i: FlagWithStrategy, allFlags = {}) => fws.inputFlag.flag.default({options: i.inputFlag.flag, flags: allFlags}) : + async () => fws.inputFlag.flag.default, + } } + + // base case (no value function) + return fws } - // set the flag metadata including the defaultHelp for that flag. - for (const k of Object.keys(this.input.flags)) { - if ((k in flags) && Reflect.has(this.input.flags[k], 'defaultHelp')) { - try { - const defaultHelpProperty = Reflect.get(this.input.flags[k], 'defaultHelp') - const defaultHelp = (typeof defaultHelpProperty === 'function' ? await defaultHelpProperty({ - options: flags[k], - flags, ...this.context, - }) : defaultHelpProperty) - this.metaData.flags[k] = {...this.metaData.flags[k], defaultHelp} - } catch { - // no-op + const addHelpFunction = (fws: FlagWithStrategy): FlagWithStrategy => { + if (fws.inputFlag.flag.type === 'option' && fws.inputFlag.flag.defaultHelp) { + return { + ...fws, helpFunction: typeof fws.inputFlag.flag.defaultHelp === 'function' ? + // @ts-expect-error flag type isn't specific enough to know defaultHelp will definitely be there + (i: FlagWithStrategy, flags: Record, ...context) => i.inputFlag.flag.defaultHelp({options: i.inputFlag, flags}, ...context) : + // @ts-expect-error flag type isn't specific enough to know defaultHelp will definitely be there + (i: FlagWithStrategy) => i.inputFlag.flag.defaultHelp, } } - } - return flags - } + return fws + } - private async _parseFlag(input: any, flag: BooleanFlag | OptionFlag, token?: FlagToken) { - if (!flag.parse) return input + const addDefaultHelp = async (fws: FlagWithStrategy[]): Promise => { + const valueReferenceForHelp = fwsArrayToObject(flagsWithAllValues.filter(fws => !fws.metadata?.setFromDefault)) + return Promise.all(fws.map(async fws => fws.helpFunction ? ({...fws, metadata: {...fws.metadata, defaultHelp: await fws.helpFunction?.(fws, valueReferenceForHelp, this.context)}}) : fws)) + } - try { - const ctx = this.context - ctx.token = token + const fwsArrayToObject = (fwsArray: FlagWithStrategy[]) => Object.fromEntries(fwsArray.map(fws => [fws.inputFlag.name, fws.value])) - if (flag.type === 'boolean') { - return await flag.parse(input, ctx, flag) + type FlagWithStrategy = { + inputFlag: { + name: string, + flag: CompletableFlag } - - return flag.parse ? await flag.parse(input, ctx, flag) : input - } catch (error: any) { - error.message = `Parsing --${flag.name} \n\t${error.message}\nSee more help with --help` - throw error + tokens?: FlagToken[], + valueFunction?: ValueFunction; + helpFunction?: (fws: FlagWithStrategy, flags: Record, ...args: any) => Promise; + metadata?: MetadataFlag + value?: any; } - } - private _validateOptions(flag: OptionFlag, input: string) { - if (flag.options && !flag.options.includes(input)) - throw new FlagInvalidOptionError(flag, input) + const flagTokenMap = this.mapAndValidateFlags() + + const flagsWithValues = await Promise.all(Object.entries(this.input.flags) + // we check them if they have a token, or might have env, default, or defaultHelp. Also include booleans so they get their default value + .filter(([name, flag]) => flag.type === 'boolean' || flag.env || flag.default || 'defaultHelp' in flag || flagTokenMap.has(name)) + // match each possible flag to its token, if there is one + .map(([name, flag]): FlagWithStrategy => ({inputFlag: {name, flag}, tokens: flagTokenMap.get(name)})) + .map(fws => addValueFunction(fws)) + .filter(fws => fws.valueFunction !== undefined) + .map(fws => addHelpFunction(fws)) + // we can't apply the default values until all the other flags are resolved because `flag.default` can reference other flags + .map(async fws => (fws.metadata?.setFromDefault ? fws : {...fws, value: await fws.valueFunction?.(fws)}))) + + const valueReference = fwsArrayToObject(flagsWithValues.filter(fws => !fws.metadata?.setFromDefault)) + + const flagsWithAllValues = await Promise.all(flagsWithValues + .map(async fws => (fws.metadata?.setFromDefault ? {...fws, value: await fws.valueFunction?.(fws, valueReference)} : fws))) + + const finalFlags = (flagsWithAllValues.some(fws => typeof fws.helpFunction === 'function')) ? await addDefaultHelp(flagsWithAllValues) : flagsWithAllValues + + return { + // @ts-ignore original version returned an any. Not sure how to get to the return type for `flags` prop + flags: fwsArrayToObject(finalFlags), + metadata: {flags: Object.fromEntries(finalFlags.filter(fws => fws.metadata).map(fws => [fws.inputFlag.name, fws.metadata as MetadataFlag]))}, + } } - private async _args(): Promise<{ argv: unknown[]; args: Record}> { + private async _args(): Promise<{ argv: unknown[]; args: Record }> { const argv: unknown[] = [] const args = {} as Record const tokens = this._argTokens @@ -409,10 +435,6 @@ export class Parser o.type === 'arg') as ArgToken[] } - private get _flagTokens(): FlagToken[] { - return this.raw.filter(o => o.type === 'flag') as FlagToken[] - } - private _setNames() { for (const k of Object.keys(this.input.flags)) { this.input.flags[k].name = k @@ -422,4 +444,19 @@ export class Parser { + const flagTokenMap = new Map() + for (const token of (this.raw.filter(o => o.type === 'flag') as FlagToken[])) { + // fail fast if there are any invalid flags + if (!(token.flag in this.input.flags)) { + throw new CLIError(`Unexpected flag ${token.flag}`) + } + + const existing = flagTokenMap.get(token.flag) ?? [] + flagTokenMap.set(token.flag, [...existing, token]) + } + + return flagTokenMap + } } diff --git a/src/parser/validate.ts b/src/parser/validate.ts index 1748e3eeb..0d6e057b3 100644 --- a/src/parser/validate.ts +++ b/src/parser/validate.ts @@ -13,6 +13,8 @@ export async function validate(parse: { input: ParserInput; output: ParserOutput; }): Promise { + let cachedResolvedFlags: Record | undefined + function validateArgs() { if (parse.output.nonExistentFlags?.length > 0) { throw new NonExistentFlagsError({parse, flags: parse.output.nonExistentFlags}) @@ -47,31 +49,35 @@ export async function validate(parse: { } async function validateFlags() { - const promises = Object.entries(parse.input.flags).map(async ([name, flag]) => { - const results: Validation[] = [] + const promises = Object.entries(parse.input.flags).flatMap(([name, flag]): Array> => { if (parse.output.flags[name] !== undefined) { - results.push( - ...await validateRelationships(name, flag), - await validateDependsOn(name, flag.dependsOn ?? []), - await validateExclusive(name, flag.exclusive ?? []), - await validateExactlyOne(name, flag.exactlyOne ?? []), - ) - } else if (flag.required) { - results.push({status: 'failed', name, validationFn: 'required', reason: `Missing required flag ${name}`}) - } else if (flag.exactlyOne && flag.exactlyOne.length > 0) { - results.push(validateAcrossFlags(flag)) + return [ + ...flag.relationships ? validateRelationships(name, flag) : [], + ...flag.dependsOn ? [validateDependsOn(name, flag.dependsOn)] : [], + ...flag.exclusive ? [validateExclusive(name, flag.exclusive)] : [], + ...flag.exactlyOne ? [validateExactlyOne(name, flag.exactlyOne)] : [], + ] + } + + if (flag.required) { + return [{status: 'failed', name, validationFn: 'required', reason: `Missing required flag ${name}`}] } - return results + if (flag.exactlyOne && flag.exactlyOne.length > 0) { + return [validateAcrossFlags(flag)] + } + + return [] }) - const results = (await Promise.all(promises)).flat() + const results = (await Promise.all(promises)) const failed = results.filter(r => r.status === 'failed') if (failed.length > 0) throw new FailedFlagValidationError({parse, failed}) } async function resolveFlags(flags: FlagRelationship[]): Promise> { + if (cachedResolvedFlags) return cachedResolvedFlags const promises = flags.map(async flag => { if (typeof flag === 'string') { return [flag, parse.output.flags[flag]] @@ -81,15 +87,11 @@ export async function validate(parse: { return result ? [flag.name, parse.output.flags[flag.name]] : null }) const resolved = await Promise.all(promises) - return Object.fromEntries(resolved.filter(r => r !== null) as [string, unknown][]) + cachedResolvedFlags = Object.fromEntries(resolved.filter(r => r !== null) as [string, unknown][]) + return cachedResolvedFlags } - function getPresentFlags(flags: Record): string[] { - return Object.keys(flags).reduce((acc, key) => { - if (flags[key] !== undefined) acc.push(key) - return acc - }, [] as string[]) - } + const getPresentFlags = (flags: Record): string[] => Object.keys(flags).filter(key => key !== undefined) function validateAcrossFlags(flag: Flag): Validation { const base = {name: flag.name, validationFn: 'validateAcrossFlags'} @@ -128,6 +130,7 @@ export async function validate(parse: { async function validateExactlyOne(name: string, flags: FlagRelationship[]): Promise { const base = {name, validationFn: 'validateExactlyOne'} + const resolved = await resolveFlags(flags) const keys = getPresentFlags(resolved) for (const flag of keys) { @@ -142,6 +145,7 @@ export async function validate(parse: { async function validateDependsOn(name: string, flags: FlagRelationship[]): Promise { const base = {name, validationFn: 'validateDependsOn'} const resolved = await resolveFlags(flags) + const foundAll = Object.values(resolved).every(val => val !== undefined) if (!foundAll) { const formattedFlags = Object.keys(resolved).map(f => `--${f}`).join(', ') @@ -153,6 +157,7 @@ export async function validate(parse: { async function validateSome(name: string, flags: FlagRelationship[]): Promise { const base = {name, validationFn: 'validateSome'} + const resolved = await resolveFlags(flags) const foundAtLeastOne = Object.values(resolved).some(Boolean) if (!foundAtLeastOne) { @@ -163,31 +168,21 @@ export async function validate(parse: { return {...base, status: 'success'} } - async function validateRelationships(name: string, flag: CompletableFlag): Promise { - if (!flag.relationships) return [] - const results = await Promise.all(flag.relationships.map(async relationship => { - const flags = relationship.flags ?? [] - const results = [] + function validateRelationships(name: string, flag: CompletableFlag): Promise[] { + return ((flag.relationships ?? []).map(relationship => { switch (relationship.type) { case 'all': - results.push(await validateDependsOn(name, flags)) - break + return validateDependsOn(name, relationship.flags) case 'some': - results.push(await validateSome(name, flags)) - break + return validateSome(name, relationship.flags) case 'none': - results.push(await validateExclusive(name, flags)) - break + return validateExclusive(name, relationship.flags) default: - break + throw new Error(`Unknown relationship type: ${relationship.type}`) } - - return results })) - - return results.flat() } validateArgs() - await validateFlags() + return validateFlags() } From 486a91e43f78e8469f8f1c5446826b2dc1737ab2 Mon Sep 17 00:00:00 2001 From: mshanemc Date: Tue, 11 Jul 2023 12:25:25 -0500 Subject: [PATCH 5/5] refactor: pr feedback --- src/parser/parse.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/parser/parse.ts b/src/parser/parse.ts index 5a45cd21e..218aa5051 100644 --- a/src/parser/parse.ts +++ b/src/parser/parse.ts @@ -275,7 +275,7 @@ export class Parser parseFlagOrThrowError(validateOptions(i.inputFlag.flag as OptionFlag, valueFromEnv), i.inputFlag.flag, this.context)} }