chore(arg-parser): include zod to yargs conversion output in snapshot #2619
Conversation
Right now, we generate the yargs config based on the zod schema introduced in 57b6add as part of argument parsing. Since in practice, the yargs config used by mongosh is static, we can use currying to do that part of the call early and include its result in the startup snapshot, removing runtime execution cost.
There was a problem hiding this comment.
Pull request overview
This PR refactors the argument parsing to enable startup snapshot optimization by currying the yargs configuration generation. The change separates schema-to-yargs conversion (which can be done at build time) from actual argument parsing (which must happen at runtime).
Key changes:
- Split
parseArgsintocreateParseArgsthat returns a curried parser function - Split
parseArgsWithCliOptionsintocreateParseArgsWithCliOptionssimilarly - Updated the CLI REPL to call the factory function at module load time, ensuring the yargs config is included in the startup snapshot
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/cli-repl/src/arg-parser.ts | Imports the new factory function and creates the parser at module load time for snapshot inclusion |
| packages/arg-parser/src/arg-parser.ts | Refactors parsing functions into factory pattern with separate types for creation vs execution options |
| packages/arg-parser/src/arg-parser.spec.ts | Adds wrapper functions to maintain backward compatibility in tests while using the new curried API |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const parseArgs = <T extends z.ZodObject>( | ||
| opts: ParserCreationOptions<T> & ArgsListOptions | ||
| ) => createParseArgs(opts)(opts); | ||
| const parseArgsWithCliOptions = <T extends z.ZodObject>( | ||
| opts: Partial<ParserCreationOptions<T>> & ArgsListOptions | ||
| ) => createParseArgsWithCliOptions(opts)(opts); |
There was a problem hiding this comment.
The test helper functions call createParseArgs and createParseArgsWithCliOptions on every test invocation, recreating the parser each time. Since these tests don't benefit from snapshot optimization and the approach differs from production usage (where the parser is created once), consider adding a comment explaining this test-only pattern to clarify why it differs from the production implementation in cli-repl/src/arg-parser.ts.
| ...options, | ||
| }); | ||
| const { _: positional, ...parsedArgs } = argv; | ||
| return ({ args }: { args: string[] }) => { |
There was a problem hiding this comment.
Recommend reviewing this with whitespace diffs turned off (https://github.com/mongodb-js/mongosh/pull/2619/changes?w=1)
gagik
left a comment
gagik
left a comment
There was a problem hiding this comment.
nice! hopefully this resolves the difference in speed
addaleax
added
the
no-title-validation
Right now, we generate the yargs config based on the zod schema introduced in 57b6add as part of argument parsing.
Since in practice, the yargs config used by mongosh is static, we can use currying to do that part of the call early and include its result in the startup snapshot, removing runtime execution cost.