-
-
Notifications
You must be signed in to change notification settings - Fork 25
feat(gunshi): add onResolveValue lifecycle hook to CliOptions #503
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
imjuni
wants to merge
6
commits into
kazupon:main
Choose a base branch
from
imjuni:feat/on-resolve-value-hook
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
cc6dbb6
feat(gunshi): add onResolveValue lifecycle hook to CliOptions
imjuni ce8cb72
fix(gunshi): pass frozen snapshot to onResolveValue hook to prevent m…
imjuni 1d4def9
fix(gunshi): recompute validationError after onResolveValue hook
imjuni c12f651
test(gunshi): assert runner is not called when onResolveValue leaves …
imjuni 9166b33
test(gunshi): spy on onResolveValue hook to assert call count and sou…
imjuni 67dd630
test(gunshi): add type tests and nested-mutation case for onResolveVa…
imjuni File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,199 @@ | ||
| /** | ||
| * @author kazuya kawaguchi (a.k.a. kazupon) | ||
| * @license MIT | ||
| */ | ||
|
|
||
| import { ArgResolveError } from 'args-tokens' | ||
| import { describe, expect, test, vi } from 'vitest' | ||
| import { revalidateError, resolveValue } from './resolver.ts' | ||
| import type { Args, ArgExplicitlyProvided, ArgValues } from 'args-tokens' | ||
|
|
||
| type TestArgs = Args & { | ||
| name: { type: 'string' } | ||
| port: { type: 'number' } | ||
| debug: { type: 'boolean' } | ||
| } | ||
|
|
||
| const values: ArgValues<TestArgs> = { | ||
| name: 'default-name', | ||
| port: 3000, | ||
| debug: false | ||
| } | ||
|
|
||
| const explicit: ArgExplicitlyProvided<TestArgs> = { | ||
| name: false, | ||
| port: false, | ||
| debug: false | ||
| } | ||
|
|
||
| describe('resolveValue', () => { | ||
| test('should return original values when hook is undefined', async () => { | ||
| const result = await resolveValue(undefined, values, explicit) | ||
| expect(result).toBe(values) | ||
| }) | ||
|
|
||
| test('should return hook result when hook returns a value', async () => { | ||
| const overridden: ArgValues<TestArgs> = { name: 'from-config', port: 8080, debug: true } | ||
| const hook = vi.fn().mockResolvedValue(overridden) | ||
|
|
||
| const result = await resolveValue(hook, values, explicit) | ||
|
|
||
| expect(result).toBe(overridden) | ||
| expect(hook).toHaveBeenCalledOnce() | ||
| // hook receives a frozen snapshot (not the original reference), but with equal values | ||
| const [calledSources] = hook.mock.calls[0] | ||
| expect(calledSources.values).toEqual(values) | ||
| expect(calledSources.values).not.toBe(values) | ||
| expect(Object.isFrozen(calledSources.values)).toBe(true) | ||
| expect(calledSources.explicit).toBe(explicit) | ||
| }) | ||
|
|
||
| test('should fall back to original values when hook returns undefined', async () => { | ||
| const hook = vi.fn().mockResolvedValue(undefined) | ||
|
|
||
| const result = await resolveValue(hook, values, explicit) | ||
|
|
||
| expect(result).toBe(values) | ||
| expect(hook).toHaveBeenCalledOnce() | ||
| }) | ||
|
|
||
| test('should pass correct sources to hook', async () => { | ||
| const explicitWithPort: ArgExplicitlyProvided<TestArgs> = { | ||
| name: false, | ||
| port: true, | ||
| debug: false | ||
| } | ||
| const hook = vi.fn().mockResolvedValue(undefined) | ||
|
|
||
| await resolveValue(hook, values, explicitWithPort) | ||
|
|
||
| const [calledSources] = hook.mock.calls[0] | ||
| expect(calledSources.values).toEqual(values) | ||
| expect(calledSources.explicit).toBe(explicitWithPort) | ||
| }) | ||
|
|
||
| test('should return original values when hook mutates snapshot and returns undefined', async () => { | ||
| const original = { name: 'original', port: 3000, debug: false } | ||
| const inputValues: ArgValues<TestArgs> = { ...original } | ||
|
|
||
| const hook = vi.fn().mockImplementation((sources: { values: ArgValues<TestArgs> }) => { | ||
| // Attempt to mutate the snapshot passed to the hook | ||
| try { | ||
| ;(sources.values as Record<string, unknown>)['name'] = 'mutated' | ||
| } catch { | ||
| // Silently ignore TypeError from frozen object in strict mode | ||
| } | ||
| return undefined | ||
| }) | ||
|
|
||
| const result = await resolveValue(hook, inputValues, explicit) | ||
|
|
||
| // Fallback must be the unmodified original | ||
| expect(result).toBe(inputValues) | ||
| expect(result.name).toBe('original') | ||
| }) | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| test('should support synchronous hook', async () => { | ||
| const overridden: ArgValues<TestArgs> = { name: 'sync-result', port: 9000, debug: true } | ||
| const hook = vi.fn().mockReturnValue(overridden) | ||
|
|
||
| const result = await resolveValue(hook, values, explicit) | ||
|
|
||
| expect(result).toBe(overridden) | ||
| }) | ||
| }) | ||
|
imjuni marked this conversation as resolved.
|
||
|
|
||
| describe('revalidateError', () => { | ||
| const requiredArgs = { | ||
| name: { type: 'string' as const, required: true as const }, | ||
| port: { type: 'number' as const } | ||
| } | ||
|
|
||
| test('should return undefined when original error is undefined', () => { | ||
| const result = revalidateError(undefined, requiredArgs, { name: 'foo', port: 3000 }) | ||
| expect(result).toBeUndefined() | ||
| }) | ||
|
|
||
| test('should clear required error when resolved value is now present', () => { | ||
| const schema = requiredArgs.name | ||
| const requiredError = new ArgResolveError( | ||
| "Optional argument '--name' is required", | ||
| 'name', | ||
| 'required', | ||
| schema | ||
| ) | ||
| const error = new AggregateError([requiredError]) | ||
|
|
||
| // hook filled in the required arg | ||
| const result = revalidateError(error, requiredArgs, { name: 'from-config', port: 3000 }) | ||
|
|
||
| expect(result).toBeUndefined() | ||
| }) | ||
|
|
||
| test('should keep required error when resolved value is still missing', () => { | ||
| const schema = requiredArgs.name | ||
| const requiredError = new ArgResolveError( | ||
| "Optional argument '--name' is required", | ||
| 'name', | ||
| 'required', | ||
| schema | ||
| ) | ||
| const error = new AggregateError([requiredError]) | ||
|
|
||
| // hook did not fill in the required arg | ||
| const result = revalidateError(error, requiredArgs, { port: 3000 } as ArgValues< | ||
| typeof requiredArgs | ||
| >) | ||
|
|
||
| expect(result).toBeInstanceOf(AggregateError) | ||
| expect(result!.errors).toHaveLength(1) | ||
| expect(result!.errors[0]).toBe(requiredError) | ||
| }) | ||
|
|
||
| test('should keep non-required errors (type, conflict) unchanged', () => { | ||
| const schema = requiredArgs.port | ||
| const typeError = new ArgResolveError( | ||
| "Optional argument '--port' should be 'number'", | ||
| 'port', | ||
| 'type', | ||
| schema | ||
| ) | ||
| const error = new AggregateError([typeError]) | ||
|
|
||
| // values look resolved but the type error should still remain | ||
| const result = revalidateError(error, requiredArgs, { name: 'foo', port: 3000 }) | ||
|
|
||
| expect(result).toBeInstanceOf(AggregateError) | ||
| expect(result!.errors).toHaveLength(1) | ||
| expect(result!.errors[0]).toBe(typeError) | ||
| }) | ||
|
|
||
| test('should partially clear errors when only some required args are resolved', () => { | ||
| const mixedArgs = { | ||
| name: { type: 'string' as const, required: true as const }, | ||
| config: { type: 'string' as const, required: true as const } | ||
| } | ||
| const nameError = new ArgResolveError( | ||
| "Optional argument '--name' is required", | ||
| 'name', | ||
| 'required', | ||
| mixedArgs.name | ||
| ) | ||
| const configError = new ArgResolveError( | ||
| "Optional argument '--config' is required", | ||
| 'config', | ||
| 'required', | ||
| mixedArgs.config | ||
| ) | ||
| const error = new AggregateError([nameError, configError]) | ||
|
|
||
| // hook filled in 'name' but not 'config' | ||
| const result = revalidateError(error, mixedArgs, { name: 'filled' } as ArgValues< | ||
| typeof mixedArgs | ||
| >) | ||
|
|
||
| expect(result).toBeInstanceOf(AggregateError) | ||
| expect(result!.errors).toHaveLength(1) | ||
| expect(result!.errors[0]).toBe(configError) | ||
| }) | ||
| }) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| /** | ||
| * @author kazuya kawaguchi (a.k.a. kazupon) | ||
| * @license MIT | ||
| */ | ||
|
|
||
| import { ArgResolveError } from 'args-tokens' | ||
| import type { Args, ArgExplicitlyProvided, ArgValues } from 'args-tokens' | ||
| import type { Awaitable, ValueResolutionSources } from './types.ts' | ||
|
|
||
| /** | ||
| * Apply the onResolveValue hook if provided, falling back to the original values. | ||
| * | ||
| * @typeParam A - The Args type from command definition | ||
| * | ||
| * @param hook - The onResolveValue hook from CLI options, if registered | ||
| * @param values - Parsed argument values with schema defaults filled in | ||
| * @param explicit - Map of which keys were explicitly provided via CLI | ||
| * @returns The resolved values from the hook, or the original values if no hook or hook returns undefined | ||
| */ | ||
| export async function resolveValue<A extends Args>( | ||
| hook: ((sources: ValueResolutionSources<A>) => Awaitable<ArgValues<A> | undefined>) | undefined, | ||
| values: ArgValues<A>, | ||
| explicit: ArgExplicitlyProvided<A> | ||
| ): Promise<ArgValues<A>> { | ||
| if (!hook) { | ||
| return values | ||
| } | ||
| // Pass a frozen shallow copy so hook cannot mutate the original values; | ||
| // the original is preserved as the fallback when the hook returns undefined. | ||
| const snapshot = Object.freeze({ ...values }) as ArgValues<A> | ||
| return (await hook({ values: snapshot, explicit })) ?? values | ||
| } | ||
|
|
||
| /** | ||
| * Recompute the validation error after the onResolveValue hook has run. | ||
| * | ||
| * Required-argument errors are dropped for any key whose value is now | ||
| * non-nullish in `resolvedValues`; all other errors (type, conflict, etc.) | ||
| * are kept unchanged. | ||
| * | ||
| * @typeParam A - The Args type from command definition | ||
| * | ||
| * @param error - The AggregateError produced by resolveArgs before the hook ran | ||
| * @param args - The Args schema used during parsing (used to map schema references back to raw keys) | ||
| * @param resolvedValues - The final values after the hook has been applied | ||
| * @returns A new AggregateError containing only the still-failing errors, or undefined if all errors are resolved | ||
| */ | ||
| export function revalidateError<A extends Args>( | ||
| error: AggregateError | undefined, | ||
| args: A, | ||
| resolvedValues: ArgValues<A> | ||
| ): AggregateError | undefined { | ||
| if (!error) return undefined | ||
|
|
||
| const values = resolvedValues as Record<string, unknown> | ||
|
|
||
| // Build a reverse map from schema object reference → rawArg key so that we | ||
| // can look up keys without having to re-apply kebab-case conversion logic. | ||
| const schemaToKey = new Map<object, string>() | ||
| for (const [rawArg, schema] of Object.entries(args)) { | ||
| schemaToKey.set(schema, rawArg) | ||
| } | ||
|
|
||
| const remaining = (error.errors as Error[]).filter(err => { | ||
| if (!(err instanceof ArgResolveError) || err.type !== 'required') return true | ||
| const rawArg = schemaToKey.get(err.schema) | ||
| if (rawArg == null) return true // Cannot match schema — keep the error conservatively | ||
| return values[rawArg] == null | ||
| }) | ||
|
|
||
| return remaining.length > 0 ? new AggregateError(remaining) : undefined | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.