feat: support lazy command for entry - #116
Conversation
WalkthroughThe CLI module was updated to support a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant LazyCommand
participant Command
participant SubCommands
User->>CLI: Invoke CLI with argv and entry (could be LazyCommand)
CLI->>CLI: resolveCommandOptions(entry)
alt entry is LazyCommand
CLI->>SubCommands: Add LazyCommand to subCommands map
end
CLI->>CLI: resolveCommand(sub, entry, options)
alt sub matches LazyCommand
CLI->>LazyCommand: Invoke lazy command
else sub matches Command
CLI->>Command: Invoke command
end
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
@gunshi/bone
@gunshi/definition
gunshi
@gunshi/plugin
@gunshi/plugin-completion
@gunshi/plugin-dryrun
@gunshi/plugin-global
@gunshi/plugin-i18n
@gunshi/plugin-renderer
@gunshi/resources
@gunshi/shared
commit: |
Deploying gunshi with
|
| Latest commit: |
8f0ba48
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c839aa92.gunshi.pages.dev |
| Branch Preview URL: | https://feat-entry-lazy-loading.gunshi.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
src/cli.ts (5)
30-33: Update JSDoc to reflect newLazyCommandparameterThe function signature now accepts
LazyCommand, but the JSDoc above (lines 25-27) still only mentionsCommandandCommandRunner.
Keeping docs in sync avoids confusion for downstream users and IDEs.- * @param entry A {@link Command | entry command} or an {@link CommandRunner | inline command runner} + * @param entry A {@link Command | entry command}, + * an {@link CommandRunner | inline command runner}, + * or a {@link LazyCommand | lazily-loaded command}
104-115: Consider always augmentingsubCommands, even when the caller did not supply anyCurrently the entry command is only injected when
options.subCommandsis truthy.
If the caller provides no sub-commands but still wishes to invoke the entry by name (e.g.cli(['lazy'], entry)), the map remains empty and the token is treated as a positional argument withcallMode = 'entry'.That may be acceptable, but it differs from the behaviour when any sub-commands are present, where
callMode = 'subCommand'.
If you want consistent semantics, populate the map unconditionally.
177-178: DefineCANNOT_RESOLVE_COMMANDas a typed const tupleWithout
as const, TypeScript widens the array to(string | undefined)[], triggering an extra cast later and losing tuple length guarantees.-const CANNOT_RESOLVE_COMMAND = [undefined, undefined, 'unexpected'] +const CANNOT_RESOLVE_COMMAND = [ + undefined, + undefined, + 'unexpected' +] as const satisfies [undefined, undefined, CommandCallMode]
187-209: Minor typing / readability tweaks fordoResolveCommand
typeof entry === 'object'is true fornull; add a null-guard.- Repeated
await resolveLazyCommand(_, '', needRunResolving)– consider extracting common helper to DRY.- The inner
if ('commandName' in entry)check reads better with a user-defined type-guard.No functional bug, but worth a cleanup pass.
211-216: Branch duplication – consolidate early-return pathsThe
if (omitted)andif (options.subCommands == null || …)blocks both returndoResolveCommand().
Pull the sub-command lookup into a single guard to reduce cognitive load:if (omitted || options.subCommands?.size === 0) { return doResolveCommand() }src/cli.test.ts (2)
68-76: Edge-case test uses empty positional tokenPassing an empty string (
['']) is not something Node/Jest normally produces fromprocess.argv.
The test is valuable for coverage, but please add a short comment explaining the intent to future readers.
88-112: Test covers the happy path – add a failure assertionYou exercise both
lazyand a real sub-command, great!
It would be useful to assert that invoking an unknown sub-command (e.g.cli(['unknown'], …)) still throws, ensuring the insertion oflazyintosubCommandsdoesn’t loosen validation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/cli.test.ts(2 hunks)src/cli.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
src/cli.ts (1)
18-20: Type import looks goodAdding
LazyCommandto the type-import list is required for the new feature and does not introduce any problems.src/cli.test.ts (2)
4-4: Import path sanity check
lazyis imported from'./definition.ts'.
If that module re-exportslazyfrom elsewhere this is fine, otherwise ensure the path is correct to avoid circular-import surprises.
78-86: Partial sub-command name may mask real typosThe test intentionally uses
'laz'(truncated) to verify fallback to the entry command.
Consider adding a negative test (['la']) or documenting that only exact matches are resolved when sub-commands exist, to avoid surprising users.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/cli.ts (1)
110-112:commandNamemay beundefined– guard before inserting into the mapIf a consumer creates a lazy command without specifying
commandName, the!assertion bypasses safety and injectsundefinedas a key:lazy(() => fn) // no name suppliedThis will silently store the command under an
undefinedkey, making it impossible to retrieve later.- if (typeof entry === 'function' && 'commandName' in entry && entry.commandName) { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - subCommands.set(entry.commandName!, entry as LazyCommand<any>) + if ( + typeof entry === 'function' && + 'commandName' in entry && + entry.commandName + ) { + subCommands.set(entry.commandName, entry as LazyCommand<any>)
🧹 Nitpick comments (4)
src/cli.ts (4)
177-177: Define explicit type for the constantThe constant
CANNOT_RESOLVE_COMMANDis using an implicit type that's later cast when used. It would be clearer to explicitly type it.-const CANNOT_RESOLVE_COMMAND = [undefined, undefined, 'unexpected'] +const CANNOT_RESOLVE_COMMAND: [undefined, undefined, CommandCallMode] = [undefined, undefined, 'unexpected']
189-208: Consider simplifying the conditional logic for better type safetyThe current implementation uses nested if-else conditionals and type assertions. Consider rewriting for better type safety and readability:
async function doResolveCommand(): Promise< [string | undefined, Command<A> | undefined, CommandCallMode] > { - if (typeof entry === 'function') { - // eslint-disable-next-line unicorn/prefer-ternary - if ('commandName' in entry) { - // lazy command - return [entry.commandName, await resolveLazyCommand(entry, '', needRunResolving), 'entry'] - } else { - // inline command (command runner) - return [undefined, { run: entry as CommandRunner<A> }, 'entry'] - } - } else if (typeof entry === 'object') { - // command object - return [ - resolveEntryName(entry), - await resolveLazyCommand(entry, '', needRunResolving), - 'entry' - ] - } else { - return CANNOT_RESOLVE_COMMAND as [string | undefined, Command<A> | undefined, CommandCallMode] - } + // Handle lazy command + if (typeof entry === 'function' && 'commandName' in entry) { + return [ + entry.commandName, + await resolveLazyCommand(entry, '', needRunResolving), + 'entry' + ] + } + + // Handle inline command runner + if (typeof entry === 'function') { + return [ + undefined, + { run: entry as CommandRunner<A> }, + 'entry' + ] + } + + // Handle command object + if (typeof entry === 'object' && entry !== null) { + return [ + resolveEntryName(entry), + entry, // No need to resolve a command object that's already resolved + 'entry' + ] + } + + // Cannot resolve + return CANNOT_RESOLVE_COMMAND }
211-223: Simplify control flow logicThe current control flow can be simplified for better readability:
- if (omitted) { - return doResolveCommand() - } else { - if (options.subCommands == null || options.subCommands.size === 0) { - return doResolveCommand() - } else { - const cmd = options.subCommands?.get(sub) - if (cmd == null) { - return [sub, undefined, 'unexpected'] - } - return [sub, await resolveLazyCommand(cmd, sub, needRunResolving), 'subCommand'] - } - } + // If command is omitted or there are no subcommands, resolve the entry + if (omitted || options.subCommands == null || options.subCommands.size === 0) { + return doResolveCommand() + } + + // Otherwise, try to resolve the subcommand + const cmd = options.subCommands?.get(sub) + if (cmd == null) { + return [sub, undefined, 'unexpected'] + } + + return [sub, await resolveLazyCommand(cmd, sub, needRunResolving), 'subCommand']
31-31: Update JSDoc to include LazyCommandThe function signature was updated to include LazyCommand, but the JSDoc comment above hasn't been updated to reflect this.
/** * Run the command. * @param args Command line arguments - * @param entry A {@link Command | entry command} or an {@link CommandRunner | inline command runner} + * @param entry A {@link Command | entry command}, an {@link CommandRunner | inline command runner}, or a {@link LazyCommand | lazy command} * @param opts A {@link CommandOptions | command options} * @returns A rendered usage or undefined. if you will use {@link CommandOptions.usageSilent} option, it will return rendered usage string. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/cli.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
src/cli.ts (1)
203-203:Details
❌ Incorrect review comment
Review the use of
resolveLazyCommandwith command objectsYou're calling
resolveLazyCommandon a command object, but based on the name and context, this function is meant for lazy commands (functions). It's unclear if this is intentional or if the command object should be used directly.
🏁 Script executed:
#!/bin/bash # Let's examine how resolveLazyCommand is implemented cat src/utils.ts | grep -A 15 "export async function resolveLazyCommand"Length of output: 559
🏁 Script executed:
#!/bin/bash # Find all occurrences of resolveLazyCommand in the codebase rg "resolveLazyCommand" -n .Length of output: 627
resolveLazyCommand usage is correct
The
resolveLazyCommandhelper is designed to accept both lazy command functions and fully constructed command objects. When passed an object, it simply returns it unchanged (aside from setting the provided name). Your calls onentryandcmdare valid—no changes needed.Likely an incorrect or invalid review comment.
1 participant
Description
Linked Issues
Additional context
Summary by CodeRabbit