-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: strengthen Options
interface
#1284
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
Conversation
antongolub
commented
Jul 23, 2025
- Tests pass
- Appropriate changes to README are included in PR
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.
Pull Request Overview
This PR strengthens the Options
interface by converting it from an interface with optional properties to a more type-safe structure using Partial<>
and introducing new helper types for better type safety and code organization.
- Restructures the
Options
interface to usePartial<>
wrapper for all properties - Introduces new type definitions (
Defaults
,NormalizedOpts
,Snapshot
) for better type organization - Adds explicit type annotations to function parameters that previously relied on default values
Reviewed Changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
src/core.ts | Refactors Options interface structure, adds new type definitions, and improves type safety with explicit parameter typing |
build/core.cjs | Updates compiled JavaScript to reflect parameter name changes from the TypeScript refactor |
.size-limit.json | Adjusts bundle size limits to accommodate the type system changes |
|
||
// prettier-ignore | ||
interface Defaults | ||
extends Required<Omit<Options, 'ac' | 'cmd' | 'cwd' | 'prefix' | 'postfix' | 'delimiter' | 'halt' | 'input' | 'quote' | 'signal' | 'store' | 'timeout'>> { |
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.
The Defaults
interface excludes 'quote' from the Required wrapper but 'quote' is included in the defaults object at line 144. This inconsistency could lead to type errors where 'quote' is expected to be always present in defaults but typed as optional.
extends Required<Omit<Options, 'ac' | 'cmd' | 'cwd' | 'prefix' | 'postfix' | 'delimiter' | 'halt' | 'input' | 'quote' | 'signal' | 'store' | 'timeout'>> { | |
extends Required<Omit<Options, 'ac' | 'cmd' | 'cwd' | 'prefix' | 'postfix' | 'delimiter' | 'halt' | 'input' | 'signal' | 'store' | 'timeout'>> { | |
quote: typeof quote |
Copilot uses AI. Check for mistakes.
// prettier-ignore | ||
export interface Shell< | ||
S = false, | ||
R = S extends true ? ProcessOutput : ProcessPromise, | ||
> { | ||
(pieces: TemplateStringsArray, ...args: any[]): R | ||
<O extends Partial<Options> = Partial<Options>, R = O extends { sync: true } ? Shell<true> : Shell>(opts: O): R | ||
<O extends Options = Options, R = O extends { sync: true } ? Shell<true> : Shell>(opts: O): R |
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.
Changing from Partial<Options>
to Options
makes the type constraint more restrictive. Since Options is now Partial<{...}>
, users may be required to provide properties that were previously optional, potentially breaking existing code.
<O extends Options = Options, R = O extends { sync: true } ? Shell<true> : Shell>(opts: O): R | |
<O extends Partial<Options> = Partial<Options>, R = O extends { sync: true } ? Shell<true> : Shell>(opts: O): R |
Copilot uses AI. Check for mistakes.
sync: { | ||
(pieces: TemplateStringsArray, ...args: any[]): ProcessOutput | ||
(opts: Partial<Omit<Options, 'sync'>>): Shell<true> | ||
(opts: Omit<Options, 'sync'>): Shell<true> |
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.
Removing Partial<>
wrapper from Omit<Options, 'sync'>
makes this parameter less flexible. Since Options properties are now all optional via Partial<>, this change may require users to provide more properties than necessary for the sync shell.
(opts: Omit<Options, 'sync'>): Shell<true> | |
(opts: Partial<Omit<Options, 'sync'>>): Shell<true> |
Copilot uses AI. Check for mistakes.
47331e3
to
e77abdb
Compare
Superseded by other refactorings |