chore: cleanup browser behavior

Author: gagik

packages/arg-parser/src/arg-parser.spec.ts

@@ -1134,10 +1134,45 @@ describe('arg-parser', function () {
   });
 
   describe('union type fields', function () {
-    for (const { argument, values, onlyFalse, strict } of [
+    describe('--browser', function () {
+      it('does not coerce to boolean with --browser=true', function () {
+        expect(
+          parseArgsWithCliOptions({ args: ['--browser=true'] }).parsed.browser
+        ).to.equal('true');
+      });
+
+      it('coerces to boolean with --browser=false', function () {
+        expect(
+          parseArgsWithCliOptions({ args: ['--browser=false'] }).parsed.browser
+        ).to.equal(false);
+      });
+
+      it('coerces to false with --no-browser', function () {
+        expect(
+          parseArgsWithCliOptions({ args: ['--no-browser'] }).parsed.browser
+        ).to.equal(false);
+      });
+
+      it('uses string if browser=something', function () {
+        expect(
+          parseArgsWithCliOptions({ args: ['--browser=something'] }).parsed
+            .browser
+        ).to.equal('something');
+      });
+
+      it('throws if just --browser is provided', function () {
+        expect(
+          () => parseArgsWithCliOptions({ args: ['--browser'] }).parsed.browser
+        ).to.throw(
+          MongoshUnimplementedError,
+          '--browser can only be true or a string'
+        );
+      });
+    });
+
+    for (const { argument, values } of [
       { argument: 'json', values: ['relaxed', 'canonical'] },
       { argument: 'oidcDumpTokens', values: ['redacted', 'include-secrets'] },
-      { argument: 'browser', values: ['test'], onlyFalse: true, strict: false },
     ] as const) {
       describe(`with ${argument}`, function () {
         context('with boolean', function () {
@@ -1149,23 +1184,13 @@ describe('arg-parser', function () {
             ).to.equal(true);
           });
 
-          if (!onlyFalse) {
-            it(`coerces to true with --${argument}=true`, function () {
-              expect(
-                parseArgsWithCliOptions({
-                  args: [`--${argument}=true`],
-                }).parsed[argument]
-              ).to.equal(true);
-            });
-          } else {
-            it(`does not coerce with "--${argument} true"`, function () {
-              expect(
-                parseArgsWithCliOptions({
-                  args: [`--${argument}=true`],
-                }).parsed[argument]
-              ).to.be.equal('true');
-            });
-          }
+          it(`coerces to true with --${argument}=true`, function () {
+            expect(
+              parseArgsWithCliOptions({
+                args: [`--${argument}=true`],
+              }).parsed[argument]
+            ).to.equal(true);
+          });
 
           it(`coerces to false with --${argument}=false`, function () {
             expect(
@@ -1197,22 +1222,16 @@ describe('arg-parser', function () {
           });
         }
 
-        if (strict) {
-          it('throws an error with invalid value', function () {
-            try {
-              parseArgsWithCliOptions({
-                args: [`--${argument}`, 'invalid'],
-              });
-            } catch (e: any) {
-              expect(e).to.be.instanceOf(MongoshUnimplementedError);
-              expect(e.message).to.include(
-                `--${argument} can only have the values ${values.join(', ')}`
-              );
-              return;
-            }
-            expect.fail('Expected error');
-          });
-        }
+        it('throws an error with invalid value', function () {
+          expect(() =>
+            parseArgsWithCliOptions({
+              args: [`--${argument}`, 'invalid'],
+            })
+          ).to.throw(
+            MongoshUnimplementedError,
+            `--${argument} can only have the values ${values.join(', ')}`
+          );
+        });
       });
     }
   });
@@ -1246,7 +1265,7 @@ describe('arg-parser', function () {
       });
     });
 
-    it('generates the expected options for Cli Options', function () {
+    it('generates the expected options for CliOptions', function () {
       const options = generateYargsOptionsFromSchema({
         schema: CliOptionsSchema,
       });
@@ -1260,6 +1279,7 @@ describe('arg-parser', function () {
           'awsIamSessionToken',
           'awsSecretAccessKey',
           'awsSessionToken',
+          'browser',
           'csfleLibraryPath',
           'cryptSharedLibPath',
           'db',
@@ -1294,6 +1314,7 @@ describe('arg-parser', function () {
           'apiDeprecationErrors',
           'apiStrict',
           'buildInfo',
+          'deepInspect',
           'exposeAsyncRewriter',
           'help',
           'ipv6',

packages/arg-parser/src/arg-parser.ts

@@ -16,6 +16,16 @@ import {
   UnsupportedCliArgumentError,
 } from './arg-metadata';
 
+function unwrapType(type: unknown): unknown {
+  if (type instanceof z.ZodOptional) {
+    return unwrapType(type.unwrap());
+  }
+  if (type instanceof z.ZodDefault) {
+    return unwrapType(type.unwrap());
+  }
+  return type;
+}
+
 /**
  * Generate yargs-parser configuration from schema
  */
@@ -50,11 +60,7 @@ export function generateYargsOptionsFromSchema({
   for (const [fieldName, fieldSchema] of Object.entries(schema.shape)) {
     const meta = getArgumentMetadata(schema, fieldName);
 
-    // Unwrap optional type
-    let unwrappedType = fieldSchema;
-    if (fieldSchema instanceof z.ZodOptional) {
-      unwrappedType = fieldSchema.unwrap();
-    }
+    const unwrappedType = unwrapType(fieldSchema);
 
     // Determine type
     if (unwrappedType instanceof z.ZodArray) {
@@ -85,6 +91,8 @@ export function generateYargsOptionsFromSchema({
         if (hasFalseLiteral) {
           // If set to 'false' coerce into false boolean; string in all other cases
           options.coerce[fieldName] = coerceIfFalse;
+          // Setting as string prevents --{field} from being valid.
+          options.string.push(fieldName);
         } else if (hasBoolean) {
           // If the field is 'true' or 'false', we coerce the value to a boolean.
           options.coerce[fieldName] = coerceIfBoolean;
@@ -107,7 +115,13 @@ export function generateYargsOptionsFromSchema({
         );
       }
     } else {
-      throw new Error(`Unknown field type: ${unwrappedType.constructor.name}`);
+      throw new Error(
+        `Unknown field type: ${
+          unwrappedType instanceof Object
+            ? unwrappedType.constructor.name
+            : typeof unwrappedType
+        }`
+      );
     }
 
     // Add aliases
@@ -253,6 +267,10 @@ export function coerceIfBoolean(value: unknown): unknown {
 }
 
 export function coerceIfFalse(value: unknown): unknown {
+  if (value === undefined || value === '') {
+    return null;
+  }
+
   if (typeof value === 'string') {
     if (value === 'false') {
       return false;

packages/arg-parser/src/cli-options.ts

@@ -197,7 +197,7 @@ export function validateCliOptions(parsed: CliOptions): void {
   const jsonValidation = CliOptionsSchema.shape.json.safeParse(parsed.json);
   if (!jsonValidation.success) {
     throw new MongoshUnimplementedError(
-      '--json can only have the values relaxed or canonical',
+      '--json can only have the values relaxed, canonical',
       CommonErrors.InvalidArgument
     );
   }
@@ -206,7 +206,7 @@ export function validateCliOptions(parsed: CliOptions): void {
     CliOptionsSchema.shape.oidcDumpTokens.safeParse(parsed.oidcDumpTokens);
   if (!oidcDumpTokensValidation.success) {
     throw new MongoshUnimplementedError(
-      '--oidcDumpTokens can only have the values redacted or include-secrets',
+      '--oidcDumpTokens can only have the values redacted, include-secrets',
       CommonErrors.InvalidArgument
     );
   }
@@ -216,7 +216,17 @@ export function validateCliOptions(parsed: CliOptions): void {
   );
   if (!jsContextValidation.success) {
     throw new MongoshUnimplementedError(
-      '--jsContext can only have the values repl, plain-vm, or auto',
+      '--jsContext can only have the values repl, plain-vm, auto',
+      CommonErrors.InvalidArgument
+    );
+  }
+
+  const browserValidation = CliOptionsSchema.shape.browser.safeParse(
+    parsed.browser
+  );
+  if (!browserValidation.success) {
+    throw new MongoshUnimplementedError(
+      '--browser can only be true or a string',
       CommonErrors.InvalidArgument
     );
   }