Fix default arguments and option when using as a module (firebase#2723)

devpeerapong · Oct 21, 2020 · 0cf70a0 · 0cf70a0
1 parent 95f01ba
commit 0cf70a0
Show file tree

Hide file tree

Showing 5 changed files with 60 additions and 40 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -0,0 +1 @@
+- Fixes incorrect defaults when using commands from Node.js (#2672)
diff --git a/README.md b/README.md
@@ -206,52 +206,44 @@ will immediately revoke access for the specified token.
 
 ## Using as a Module
 
-The Firebase CLI can also be used programmatically as a standard Node module. Each command is exposed as a function that takes an options object and returns a Promise. For example:
+The Firebase CLI can also be used programmatically as a standard Node module.
+Each command is exposed as a function that takes positional arguments followed
+by an options object and returns a Promise.
 
-```js
-var client = require("firebase-tools");
-client.projects
-  .list()
-  .then(function(data) {
-    console.log(data);
-  })
-  .catch(function(err) {
-    // handle error
-  });
+So if we run this command at our command line:
 
-client
-  .deploy({
-    project: "myfirebase",
-    token: process.env.FIREBASE_TOKEN,
-    force: true,
-    cwd: "/path/to/project/folder",
-  })
-  .then(function() {
-    console.log("Rules have been deployed!");
-  })
-  .catch(function(err) {
-    // handle error
-  });
+```bash
+$ firebase --project="foo" apps:list ANDROID
 ```
 
-Some commands, such as `firebase use` require both positional arguments and options flags. In this case you first
-provide any positional arguments as strings followed by an object containing the options:
+That translates to the following in Node:
 
 ```js
-var client = require("firebase-tools");
-client
-  .use("projectId", {
-    // Equivalent to --add when using the CLI
-    add: true,
+const client = require("firebase-tools");
+client.apps
+  .list("ANDROID", { project: "foo" })
+  .then((data) => {
+    // ...
   })
-  .then(function(data) {
-    console.log(data);
-  })
-  .catch(function(err) {
-    // handle error
+  .catch((err) => {
+    // ...
   });
 ```
 
+The options object must be the very last argument and any unspecified
+positional argument will get the default value of `""`. The following
+two invocations are equivalent:
+
+```js
+const client = require("firebase-tools");
+
+// #1 - No arguments or options, defaults will be inferred
+client.apps.list();
+
+// #2 - Explicitly provide "" for all arguments and {} for options
+client.apps.list("", {});
+```
+
 Note: when used in a limited environment like Cloud Functions, not all `firebase-tools` commands will work programatically
 because they require access to a local filesystem.
 

diff --git a/scripts/client-integration-tests/tests.ts b/scripts/client-integration-tests/tests.ts
@@ -50,6 +50,21 @@ describe("deployHosting", () => {
 });
 
 describe("apps:list", () => {
+  it("should be able to list apps with missing or undefined optional arguments", async () => {
+    const noArgsApps = await client.apps.list({ project: process.env.FBTOOLS_TARGET_PROJECT });
+    expect(noArgsApps).to.have.length.greaterThan(0);
+
+    const undefinedArgsApps = await client.apps.list(undefined, {
+      project: process.env.FBTOOLS_TARGET_PROJECT,
+    });
+    expect(undefinedArgsApps).to.have.length.greaterThan(0);
+
+    const nullArgsApps = await client.apps.list(null, {
+      project: process.env.FBTOOLS_TARGET_PROJECT,
+    });
+    expect(nullArgsApps).to.have.length.greaterThan(0);
+  });
+
   it("should list apps configuration", async () => {
     const apps = await client.apps.list("web", { project: process.env.FBTOOLS_TARGET_PROJECT });
 

diff --git a/src/command.ts b/src/command.ts
@@ -38,6 +38,7 @@ export class Command {
   private befores: BeforeFunction[] = [];
   private helpText = "";
   private client?: CLIClient;
+  private positionalArgs: { name: string; required: boolean }[] = [];
 
   /**
    * @param cmd the command to create.
@@ -132,6 +133,9 @@ export class Command {
       });
     }
 
+    // See below about using this private property
+    this.positionalArgs = cmd._args;
+
     // args is an array of all the arguments provided for the command PLUS the
     // options object as provided by Commander (on the end).
     // eslint-disable-next-line @typescript-eslint/no-explicit-any
@@ -280,10 +284,18 @@ export class Command {
   runner(): (...a: any[]) => Promise<void> {
     // eslint-disable-next-line @typescript-eslint/no-explicit-any
     return async (...args: any[]) => {
-      // always provide at least an empty object for options
-      if (args.length === 0) {
+      // Make sure the last argument is an object for options, add {} if none
+      if (typeof last(args) !== "object" || last(args) === null) {
         args.push({});
       }
+
+      // Args should have one entry for each positional arg (even the optional
+      // ones) and end with options.
+      while (args.length < this.positionalArgs.length + 1) {
+        // Add "" for missing args while keeping options at the end
+        args.splice(args.length - 1, 0, "");
+      }
+
       const options = last(args);
       this.prepare(options);
       for (const before of this.befores) {

diff --git a/src/commands/apps-list.ts b/src/commands/apps-list.ts
@@ -39,9 +39,9 @@ module.exports = new Command("apps:list [platform]")
   )
   .before(requireAuth)
   .action(
-    async (platform: string = "", options: any): Promise<AppMetadata[]> => {
+    async (platform: string | undefined, options: any): Promise<AppMetadata[]> => {
       const projectId = getProjectId(options);
-      const appPlatform = getAppPlatform(platform);
+      const appPlatform = getAppPlatform(platform || "");
 
       let apps;
       const spinner = ora(