Remove 'request' from 'functions:shell' (#5808)

* removing request and api from functions shell

* typing is powerful

* return sentinal value on http function to avoid weird output

* fix get/post functions to accept data

* spelling is hard

* Cleaning up old code

* Replace request with a shim

* refactoring to pipe through body correctly

* Use correct function signature

* format

* format

* remove overly specific error message

* remove request from storage-integration-test

* formats

---------

Co-authored-by: Bryan Kendall <[email protected]>

package.json

@@ -135,7 +135,6 @@
  "portfinder": "^1.0.32",
  "progress": "^2.0.3",
  "proxy-agent": "^6.3.0",
- "request": "^2.87.0",
  "retry": "^0.13.1",
  "rimraf": "^3.0.0",
  "semver": "^7.5.2",
@@ -189,7 +188,6 @@
  "@types/puppeteer": "^5.4.2",
  "@types/react": "^18.0.20",
  "@types/react-dom": "^18.0.6",
- "@types/request": "^2.48.1",
  "@types/retry": "^0.12.1",
  "@types/rimraf": "^2.0.3",
  "@types/semver": "^6.0.0",

scripts/storage-emulator-integration/utils.ts

@@ -1,6 +1,6 @@
 import * as fs from "fs";
 import * as path from "path";
-import * as request from "request";
+import fetch from "node-fetch";
 import * as crypto from "crypto";
 import * as os from "os";
 import { FrameworkOptions } from "../integration-helpers/framework";
@@ -81,11 +81,7 @@ export function writeToFile(filename: string, contents: Buffer, tmpDir: string):
  * Resets the storage layer of the Storage Emulator.
  */
 export async function resetStorageEmulator(emulatorHost: string) {
- await new Promise<void>((resolve) => {
- request.post(`${emulatorHost}/internal/reset`, () => {
- resolve();
- });
- });
+ await fetch(`${emulatorHost}/internal/reset`, { method: "POST" });
 }
 
 export async function getProdAccessToken(serviceAccountKey: any): Promise<string> {

src/apiv2.ts

@@ -20,7 +20,7 @@ const CLI_VERSION: string = pkg.version;
 
 const GOOG_QUOTA_USER = "x-goog-quota-user";
 
-export type HttpMethod = "GET" | "PUT" | "POST" | "DELETE" | "PATCH";
+export type HttpMethod = "GET" | "PUT" | "POST" | "DELETE" | "PATCH" | "OPTIONS" | "HEAD";
 
 interface BaseRequestOptions<T> extends VerbOptions {
  method: HttpMethod;
@@ -184,6 +184,13 @@ export class Client {
  return this.request<unknown, ResT>(reqOptions);
  }
 
+ options<ResT>(path: string, options: ClientVerbOptions = {}): Promise<ClientResponse<ResT>> {
+ const reqOptions: ClientRequestOptions<unknown> = Object.assign(options, {
+ method: "OPTIONS",
+ path,
+ });
+ return this.request<unknown, ResT>(reqOptions);
+ }
  /**
  * Makes a request as specified by the options.
  * By default, this will:

src/deploy/functions/runtimes/discovery/index.ts

@@ -1,4 +1,4 @@
-import fetch from "node-fetch";
+import fetch, { Response } from "node-fetch";
 import * as fs from "fs";
 import * as path from "path";
 import * as yaml from "js-yaml";
@@ -71,8 +71,7 @@ export async function detectFromPort(
  runtime: runtimes.Runtime,
  timeout = 10_000 /* 10s to boot up */
 ): Promise<build.Build> {
- // The result type of fetch isn't exported
- let res: { text(): Promise<string>; status: number };
+ let res: Response;
  const timedOut = new Promise<never>((resolve, reject) => {
  setTimeout(() => {
  reject(new FirebaseError("User code failed to load. Cannot determine backend specification"));

src/functionsShellCommandAction.ts

@@ -2,7 +2,6 @@
 import * as clc from "colorette";
 import * as repl from "repl";
 import * as _ from "lodash";
-import * as request from "request";
 import * as util from "util";
 
 import * as shell from "./emulator/functionsEmulatorShell";
@@ -14,8 +13,9 @@ import { logger } from "./logger";
 import { EMULATORS_SUPPORTED_BY_FUNCTIONS, EmulatorInfo, Emulators } from "./emulator/types";
 import { EmulatorHubClient } from "./emulator/hubClient";
 import { resolveHostAndAssignPorts } from "./emulator/portUtils";
-import { Options } from "./options";
 import { Constants } from "./emulator/constants";
+import { Options } from "./options";
+import { HTTPS_SENTINEL } from "./localFunction";
 import { needProjectId } from "./projectUtils";
 
 const serveFunctions = new FunctionsServer();
@@ -125,10 +125,8 @@ export const actionFunction = async (options: Options) => {
  );
 
  const writer = (output: any) => {
- // Prevent full print out of Request object when a request is made
- // @ts-ignore
- if (output instanceof request.Request) {
- return "Sent request to function.";
+ if (output === HTTPS_SENTINEL) {
+ return HTTPS_SENTINEL;
  }
  return util.inspect(output);
  };

src/localFunction.ts

@@ -1,11 +1,14 @@
 import * as uuid from "uuid";
-import * as request from "request";
 
 import { encodeFirestoreValue } from "./firestore/encodeFirestoreValue";
 import * as utils from "./utils";
 import { EmulatedTriggerDefinition } from "./emulator/functionsEmulatorShared";
 import { FunctionsEmulatorShell } from "./emulator/functionsEmulatorShell";
 import { AuthMode, AuthType, EventOptions } from "./emulator/events/types";
+import { Client, ClientResponse, ClientVerbOptions } from "./apiv2";
+
+// HTTPS_SENTINEL is sent when a HTTPS call is made via functions:shell.
+export const HTTPS_SENTINEL = "Request sent to function.";
 
 /**
  * LocalFunction produces EmulatedTriggerDefinition into a function that can be called inside the nodejs repl.
@@ -33,25 +36,119 @@ export default class LocalFunction {
  });
  }
 
- private constructCallableFunc(
- data: string | object,
- opts: { instanceIdToken?: string }
- ): request.Request {
+ private constructCallableFunc(data: string | object, opts: { instanceIdToken?: string }): void {
  opts = opts || {};
 
  const headers: Record<string, string> = {};
  if (opts.instanceIdToken) {
  headers["Firebase-Instance-ID-Token"] = opts.instanceIdToken;
  }
 
- return request.post({
- callback: (...args) => this.requestCallBack(...args),
- baseUrl: this.url,
- uri: "",
- body: { data },
- json: true,
- headers: headers,
+ if (!this.url) {
+ throw new Error("No URL provided");
+ }
+
+ const client = new Client({ urlPrefix: this.url, auth: false });
+ void client
+ .post<any, any>("", data, { headers })
+ .then((res) => {
+ this.requestCallBack<any>(undefined, res, res.body);
+ })
+ .catch((err) => {
+ this.requestCallBack(err);
+ });
+ }
+
+ private constructHttpsFunc(): requestShim {
+ if (!this.url) {
+ throw new Error("No URL provided");
+ }
+ const callClient = new Client({ urlPrefix: this.url, auth: false });
+ type verbFn = (...args: any) => Promise<string>;
+ const verbFactory = (
+ hasRequestBody: boolean,
+ method: (
+ path: string,
+ bodyOrOpts?: any,
+ opts?: ClientVerbOptions
+ ) => Promise<ClientResponse<any>>
+ ): verbFn => {
+ return async (pathOrOptions?: string | HttpsOptions, options?: HttpsOptions) => {
+ const { path, opts } = this.extractArgs(pathOrOptions, options);
+ try {
+ const res = hasRequestBody
+ ? await method(path, opts.body, toClientVerbOptions(opts))
+ : await method(path, toClientVerbOptions(opts));
+ this.requestCallBack(undefined, res, res.body);
+ } catch (err) {
+ this.requestCallBack(err);
+ }
+ return HTTPS_SENTINEL;
+ };
+ };
+
+ const shim = verbFactory(true, (path: string, json?: any, opts?: ClientVerbOptions) => {
+ const req = Object.assign(opts || {}, {
+ path: path,
+ body: json,
+ method: opts?.method || "GET",
+ });
+ return callClient.request(req);
  });
+ const verbs: verbMethods = {
+ post: verbFactory(true, (path: string, json?: any, opts?: ClientVerbOptions) =>
+ callClient.post(path, json, opts)
+ ),
+ put: verbFactory(true, (path: string, json?: any, opts?: ClientVerbOptions) =>
+ callClient.put(path, json, opts)
+ ),
+ patch: verbFactory(true, (path: string, json?: any, opts?: ClientVerbOptions) =>
+ callClient.patch(path, json, opts)
+ ),
+ get: verbFactory(false, (path: string, opts?: ClientVerbOptions) =>
+ callClient.get(path, opts)
+ ),
+ del: verbFactory(false, (path: string, opts?: ClientVerbOptions) =>
+ callClient.delete(path, opts)
+ ),
+ delete: verbFactory(false, (path: string, opts?: ClientVerbOptions) =>
+ callClient.delete(path, opts)
+ ),
+ options: verbFactory(false, (path: string, opts?: ClientVerbOptions) =>
+ callClient.options(path, opts)
+ ),
+ };
+ return Object.assign(shim, verbs);
+ }
+
+ private extractArgs(
+ pathOrOptions?: string | HttpsOptions,
+ options?: HttpsOptions
+ ): { path: string; opts: HttpsOptions } {
+ // Case: No arguments provided
+ if (!pathOrOptions && !options) {
+ return { path: "/", opts: {} };
+ }
+
+ // Case: pathOrOptions is provided as a string
+ if (typeof pathOrOptions === "string") {
+ return { path: pathOrOptions, opts: options || {} };
+ }
+
+ // Case: pathOrOptions is an object (HttpsOptions), and options is not provided
+ if (typeof pathOrOptions !== "string" && !!pathOrOptions && !options) {
+ return { path: "/", opts: pathOrOptions };
+ }
+
+ // Error case: Invalid combination of arguments
+ if (typeof pathOrOptions !== "string" || !options) {
+ throw new Error(
+ `Invalid argument combination: Expected a string and/or HttpsOptions, got ${typeof pathOrOptions} and ${typeof options}`
+ );
+ }
+
+ // Default return, though this point should not be reached
+ return { path: "/", opts: {} };
  }
 
  constructAuth(auth?: EventOptions["auth"], authType?: AuthType): AuthMode {
@@ -120,11 +217,11 @@ export default class LocalFunction {
  };
  }
 
- private requestCallBack(err: unknown, response: request.Response, body: string | object) {
+ private requestCallBack<T>(err: unknown, response?: ClientResponse<T>, body?: string | object) {
  if (err) {
  return console.warn("\nERROR SENDING REQUEST: " + err);
  }
- const status = response ? response.statusCode + ", " : "";
+ const status = response ? response.status + ", " : "";
 
  // If the body is a string we want to check if we can parse it as JSON
  // and pretty-print it. We can't blindly stringify because stringifying
@@ -240,14 +337,46 @@ export default class LocalFunction {
  if (isCallable) {
  return (data: any, opt: any) => this.constructCallableFunc(data, opt);
  } else {
- return request.defaults({
- callback: (...args) => this.requestCallBack(...args),
- baseUrl: this.url,
- uri: "",
- });
+ return this.constructHttpsFunc();
  }
  } else {
  return (data: any, opt: any) => this.triggerEvent(data, opt);
  }
  }
 }
+
+// requestShim is a minimal implementation of the public API of the deprecated `request` package
+// We expose it as part of `functions:shell` so that we can keep the previous API while removing
+// our dependency on `request`.
+interface requestShim extends verbMethods {
+ (...args: any): any;
+ // TODO(taeold/blidd/joehan) What other methods do we need to add? form? json? multipart?
+}
+
+interface verbMethods {
+ get(...args: any): any;
+ post(...args: any): any;
+ put(...args: any): any;
+ patch(...args: any): any;
+ del(...args: any): any;
+ delete(...args: any): any;
+ options(...args: any): any;
+}
+
+// HttpsOptions is a subset of request's CoreOptions
+// https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/request/index.d.ts#L107
+// We intentionally omit options that are likely useless for `functions:shell`
+type HttpsOptions = {
+ method?: "GET" | "PUT" | "POST" | "DELETE" | "PATCH" | "OPTIONS" | "HEAD";
+ headers?: Record<string, any>;
+ body?: any;
+ qs?: any;
+};
+
+function toClientVerbOptions(opts: HttpsOptions): ClientVerbOptions {
+ return {
+ method: opts.method,
+ headers: opts.headers,
+ queryParams: opts.qs,
+ };
+}

src/test/apiv2.spec.ts

@@ -158,7 +158,7 @@ describe("apiv2", () => {
  method: "GET",
  path: "/path/to/foo",
  });
- await expect(r).to.eventually.be.rejectedWith(FirebaseError, /Unable to parse JSON/);
+ await expect(r).to.eventually.be.rejectedWith(FirebaseError);
  expect(nock.isDone()).to.be.true;
  });