-
Notifications
You must be signed in to change notification settings - Fork 70
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
Prerelease/v2 #588
Prerelease/v2 #588
Conversation
* feat: improve interfaces * feat: new command types * feat: more changes * chore: clean up * chore: enable eslint rules * chore: add files * feat: remove parser/deps * chore: more clean up * chore: tests * feat: rename globalFlags to baseFlags * feat: typed args * chore: renaming things and cleanup * chore: make tests compilable * chore: add args types test * chore: get tests passing * feat: flatten ux * fix: ux.table export * fix: read from stdin * fix: args and cleanup * chore: tests * fix: arg and flag types * fix: small bugs * fix: handle non-existent flags * chore: tests * chore: compilation issues * chore: update cli-ux docs * feat: add execute * refactor: clean up cli-ux imports * test: begin removing fancy-test * feat: remove ux.open * chore: cleam up from merge * feat: add delimiter option * fix: deprecateAliases * chore: update tests * chore: update migration guide * fix: add backwards compatability for v1 args * fix: typing for default context * fix: styledObject type and boolean flag parser * fix: types
}, | ||
}) | ||
|
||
export const directory = custom<string, {exists?: boolean}>({ |
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.
@mdonnalley should we also allow access verification to dir and file as an option?
parse: async b => Boolean(b) && isNotFalsy(b), | ||
}) | ||
|
||
export const integer = custom<number, {min?: number; max?: number;}>({ |
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.
@mdonnalley what about decimal numbers?
@@ -195,7 +195,7 @@ export class ActionBase { | |||
} | |||
|
|||
// write to the real stdout/stderr | |||
protected _write(std: 'stdout' | 'stderr', s: string | string[]) { | |||
protected _write(std: 'stdout' | 'stderr', s: string | string[]): void { | |||
this.stdmockOrigs[std].apply(process[std], castArray(s) as [string]) |
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.
Why is the assertion of as string[] necessary?
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.
If you remove it, you get this:
(alias) castArray<string>(input?: string | string[] | undefined): string[]
import castArray
Argument of type 'string[]' is not assignable to parameter of type '[str: string | Uint8Array, encoding?: BufferEncoding | undefined, cb?: ((err?: Error | undefined) => void) | undefined]'.
Target requires 1 element(s) but source may have fewer.ts(2345)
My understanding is that it means the function expects an array of exactly one element, which isn't guaranteed by string[]
, hence the need to do as [string]
Personally, this part of the code base falls under the "if it ain't broke don't fix it" category so I'd prefer to leave it as is
const cmd = new this(argv, config) | ||
return cmd._run(argv) | ||
if (!cmd.id) { |
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.
When does a command not have an Id?
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.
@peternhale This is something that Shane discovered while trying to write command-level unit tests without @oclif/test. The command lacks an id
anytime you use the static run
method, like in this example: https://github.com/oclif/core#usage
In the normal code path, the id
is determined based on the file path containing the command class but in this use case, there's no file path so there's no way to determine what the id
should be.
In the real world most developers using the static run
would add the id
prop to their command so this code block would rarely be used. But it's helpful when writing unit tests for commands that aren't mean to be run using the static run
method.
if (!this.jsonEnabled()) Errors.warn(input) | ||
return input | ||
} | ||
|
||
error(input: string | Error, options: {code?: string; exit: false} & PrettyPrintableError): void | ||
public error(input: string | Error, options: {code?: string; exit: false} & PrettyPrintableError): void |
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.
Can conditional types help with these function overloading?
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.
Did you have something specific in mind? I don't think a conditional type would eliminate the need to overload this method. You still need to express that if {exit: false}
is provided, then return type is void
otherwise it's never
src/config/config.ts
Outdated
@@ -734,8 +712,28 @@ const defaultToCached = async (flag: CompletableOptionFlag<any>) => { | |||
} | |||
} | |||
|
|||
export async function toCached(c: Command.Class, plugin?: IPlugin): Promise<Command> { | |||
const flags = {} as {[k: string]: Command.Flag} | |||
const defaultArgToCached = async (arg: Arg<any>) => { |
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.
Return type is missing
} as BooleanFlag<T> | ||
} | ||
|
||
export const integer = custom<number, {min?: number; max?: number;}>({ |
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.
consider number flag?
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.
@peternhale Is there currently a need for it? Generally speaking, I prefer to wait until we have a real use case before adding a new feature
}) | ||
|
||
export const file = custom<string, {exists?: boolean}>({ | ||
parse: async (input, _, opts) => { |
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.
Consider access checks as an option
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.
@peternhale same question: is there currently a need for it?
src/util.ts
Outdated
} | ||
|
||
export function isTruthy(input: string): boolean { | ||
return ['true', 'TRUE', '1', 'yes', 'YES', 'y', 'Y'].includes(input) |
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.
This cannot handle True
| Yes
. I would suggest testing the lower case of input
src/util.ts
Outdated
} | ||
|
||
export function isNotFalsy(input: string): boolean { | ||
return !['false', 'FALSE', '0', 'no', 'NO', 'n', 'N'].includes(input) |
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.
Consider lower case of input here too.
Breaking Changes
Command Args
We updated the
Command.args
to more closely resemble flagsBefore
After
These are the available Args:
Interfaces
Interfaces.Command
since they were not usable for tests. These are replaced by types that are available under theCommand
namespaceCliUx
We flattened
CliUx.ux
intoux
for ease of useBefore
After
CliUx.ux.open
We removed the
open
method since it was a direct import/export of theopen
package. If you need this functionality, then you should importopen
yourself.Flags
color
flagglobalFlags
tobaseFlags
globalFlags
was a misleading name because the flags added there weren't actually global to the entire CLI. Instead, they were just flags that would be inherited by any command that extended the command class they were defined in.Flag and Arg Parsing
--hekp
were parsed as arguments, instead of throwing an error. In order to handle this, oclif now assumes that anything that starts with a hypen must be a flag and will throw an error if no corresponding flag definition is found. In other words, your command can no longer accept arguments that begin with a hyphen (fixes Addingstrict=false
parses a non-existing flag as aarg
#526)parse
. This was added to support backwards compatibility for flags that separated values by commas (e.g.my-flag=val1,val2
). However, this was problematic because it didn't allow theparse
to manipulate the individual values. If you need this functionality, you can now set adelimiter
option on your flags. By doing so, oclif will split the string on the delimiter before parsing.ESM/CJS Friendliness
Writing plugins with ESM has always been possible, but it requires a handful of modifications for it to work, especially in the bin scripts. In v2 we've introduced an
execute
method that the bin scripts can use to avoid having to make changes for ESM of CJS.CJS
bin/dev
beforeCJS
bin/dev.js
afterESM
bin/dev.js
beforeESM
bin/dev.js
afterNote that ESM and CJS plugins still require different settings in the tsconfig.json - you will still need to make those modifications yourself.
Other Changes
@oclif/screen
@oclif/linewrap
withwordwrap
deprecated
property does not work when=
is used to provide value #586TODO
ux.open
method (users can import theopen
library directly if they need it)will-fix-in-v2
issuesPromiseLike
Related
oclif/oclif.github.io#188