-
-
Notifications
You must be signed in to change notification settings - Fork 25
fix: make error type guards work across duplicated class copies #678
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,5 @@ | ||
| [default.extend-words] | ||
| # Intentional misspelling used in strict unknown-option tests and docs. | ||
| alow = "alow" | ||
| # Intentional misspelling used in command-not-found tests. | ||
| lod = "lod" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| import { ArgsValidationError, ArgsValidationErrorKeys } from 'args-tokens' | ||
| import { describe, expect, test } from 'vitest' | ||
| import { | ||
| CommandNotFoundError, | ||
| hasPriorityValidationError, | ||
| isArgsValidationError, | ||
| isCommandNotFoundError | ||
| } from './error.ts' | ||
|
|
||
| /** | ||
| * Stand-ins for the duplicated class copies that `@gunshi/plugin` ships: it is bundled | ||
| * with `noExternal: ['gunshi/plugin']`, so a plugin importing these guards holds a | ||
| * different class object than the one `gunshi` throws with, and `instanceof` cannot match. | ||
| */ | ||
| class DuplicatedCommandNotFoundError extends Error { | ||
| readonly commandName: string | ||
| readonly candidates: readonly string[] | ||
| constructor(message: string, commandName: string, candidates: readonly string[]) { | ||
| super(message) | ||
| this.name = 'CommandNotFoundError' | ||
| this.commandName = commandName | ||
| this.candidates = candidates | ||
| } | ||
| } | ||
|
|
||
| class DuplicatedArgsValidationError extends Error { | ||
| readonly code: string | ||
| readonly values: Record<string, unknown> | ||
| constructor(message: string, code: string, values: Record<string, unknown>) { | ||
| super(message) | ||
| this.name = 'ArgsValidationError' | ||
| this.code = code | ||
| this.values = values | ||
| } | ||
| } | ||
|
|
||
| describe('isCommandNotFoundError', () => { | ||
| test('matches an instance of the class', () => { | ||
| const error = new CommandNotFoundError('not found', { commandName: 'lod' }) | ||
| expect(isCommandNotFoundError(error)).toBe(true) | ||
| }) | ||
|
|
||
| test('matches an error from a duplicated copy of the class', () => { | ||
| const error = new DuplicatedCommandNotFoundError('not found', 'lod', ['load']) | ||
| expect(error instanceof CommandNotFoundError).toBe(false) | ||
| expect(isCommandNotFoundError(error)).toBe(true) | ||
| }) | ||
|
|
||
| test('does not match unrelated errors or non-errors', () => { | ||
| expect(isCommandNotFoundError(new Error('boom'))).toBe(false) | ||
| expect(isCommandNotFoundError({ name: 'CommandNotFoundError' })).toBe(false) | ||
| expect(isCommandNotFoundError(undefined)).toBe(false) | ||
| }) | ||
| }) | ||
|
|
||
| describe('isArgsValidationError', () => { | ||
| test('matches an instance of the class', () => { | ||
| const error = new ArgsValidationError('unknown option', { | ||
| code: ArgsValidationErrorKeys.unknownOption, | ||
| values: { name: 'alow-reload' } | ||
| }) | ||
| expect(isArgsValidationError(error)).toBe(true) | ||
| }) | ||
|
|
||
| test('matches an error from a duplicated copy of the class', () => { | ||
| const error = new DuplicatedArgsValidationError( | ||
| 'unknown option', | ||
| ArgsValidationErrorKeys.unknownOption, | ||
| { name: 'alow-reload' } | ||
| ) | ||
| expect(error instanceof ArgsValidationError).toBe(false) | ||
| expect(isArgsValidationError(error)).toBe(true) | ||
| }) | ||
|
|
||
| test('does not match unrelated errors or non-errors', () => { | ||
| expect(isArgsValidationError(new Error('boom'))).toBe(false) | ||
| expect(isArgsValidationError({ name: 'ArgsValidationError' })).toBe(false) | ||
| expect(isArgsValidationError(undefined)).toBe(false) | ||
| }) | ||
| }) | ||
|
|
||
| describe('hasPriorityValidationError', () => { | ||
| test('detects a duplicated-copy unknown-option error', () => { | ||
| const error = new AggregateError([ | ||
| new DuplicatedArgsValidationError('unknown', ArgsValidationErrorKeys.unknownOption, {}) | ||
| ]) | ||
| expect(hasPriorityValidationError(error)).toBe(true) | ||
| }) | ||
|
|
||
| test('detects a duplicated-copy command-not-found error', () => { | ||
| const error = new AggregateError([ | ||
| new DuplicatedCommandNotFoundError('not found', 'lod', ['load']) | ||
| ]) | ||
| expect(hasPriorityValidationError(error)).toBe(true) | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,12 @@ | |
| * @license MIT | ||
| */ | ||
|
|
||
| import { ArgsValidationErrorKeys, isArgsValidationError } from 'args-tokens' | ||
| import { | ||
| ArgsValidationErrorKeys, | ||
| isArgsValidationError as isArgsValidationErrorInstance | ||
| } from 'args-tokens' | ||
|
|
||
| import type { ArgsValidationError } from 'args-tokens' | ||
|
|
||
| /** | ||
| * Command not found error resource keys. | ||
|
|
@@ -82,7 +87,37 @@ export class CommandNotFoundError extends Error { | |
| * @returns `true` if the error is a {@link CommandNotFoundError} | ||
| */ | ||
| export function isCommandNotFoundError(error: unknown): error is CommandNotFoundError { | ||
| return error instanceof CommandNotFoundError | ||
| return ( | ||
| error instanceof CommandNotFoundError || | ||
| // `instanceof` alone is not enough: `@gunshi/plugin` is bundled with its own copy of | ||
| // this class (`noExternal: ['gunshi/plugin']`), so an error thrown by `gunshi` is never | ||
| // an instance of the class a plugin imports. Fall back to a structural check on the | ||
| // `name` brand the constructor sets, so the guard works across duplicated copies. | ||
| (error instanceof Error && | ||
| error.name === 'CommandNotFoundError' && | ||
| 'commandName' in error && | ||
| 'candidates' in error) | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we validate the property values here rather than only checking that the keys exist? For example, this currently passes the guard: Object.assign(new Error('bad'), {
name: 'CommandNotFoundError',
commandName: 'x',
candidates: undefined
})
At minimum, could we check |
||
| ) | ||
| } | ||
|
|
||
| /** | ||
| * Check whether an error is an {@link ArgsValidationError}. | ||
| * | ||
| * Prefer this over the `args-tokens` guard of the same name: it additionally matches | ||
| * errors produced by a duplicated copy of the class, which is what plugins importing | ||
| * from `@gunshi/plugin` receive. | ||
| * | ||
| * @param error - An unknown error | ||
| * @returns `true` if the error is an {@link ArgsValidationError} | ||
| */ | ||
| export function isArgsValidationError(error: unknown): error is ArgsValidationError { | ||
| return ( | ||
| isArgsValidationErrorInstance(error) || | ||
| (error instanceof Error && | ||
| error.name === 'ArgsValidationError' && | ||
| 'code' in error && | ||
| 'values' in error) | ||
| ) | ||
| } | ||
|
|
||
| /** | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Fix the failing typo-check fixture values.
lodfails the repository typo check on each listed line. Use a non-dictionary invalid command such asunknown-command, or add an explicit typo-check exception if this spelling is required for the test.Proposed fix
Also applies to: 44-44, 92-92
🧰 Tools
🪛 GitHub Actions: Typos / 0_Spell check with Typos.txt
[error] 39-39: Typos check failed:
lodshould beload.🪛 GitHub Actions: Typos / Spell check with Typos
[error] 39-39: Typos check failed in './typos .':
lodshould beload.🪛 GitHub Check: Spell check with Typos
[warning] 39-39:
"lod" should be "load".
🤖 Prompt for AI Agents
Sources: Linters/SAST tools, Pipeline failures