Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 21 commits into from
Nov 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
610 changes: 4 additions & 606 deletions npm-shrinkwrap.json

Large diffs are not rendered by default.

2 changes: 0 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down
8 changes: 2 additions & 6 deletions scripts/storage-emulator-integration/utils.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -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> {
Expand Down
9 changes: 8 additions & 1 deletion src/apiv2.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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:
Expand Down
5 changes: 2 additions & 3 deletions src/deploy/functions/runtimes/discovery/index.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -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"));
Expand Down
10 changes: 4 additions & 6 deletions src/functionsShellCommandAction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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();
Expand Down Expand Up @@ -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);
};
Expand Down
167 changes: 148 additions & 19 deletions src/localFunction.ts
Original file line number Diff line number Diff line change
@@ -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.";
joehan marked this conversation as resolved.
Show resolved Hide resolved

/**
* LocalFunction produces EmulatedTriggerDefinition into a function that can be called inside the nodejs repl.
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q: can this be:

callClient.post.bind(callClient))

instead? (to save some characters)

),
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(
joehan marked this conversation as resolved.
Show resolved Hide resolved
`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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
};
}
2 changes: 1 addition & 1 deletion src/test/apiv2.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
});

Expand Down
Loading