Skip to content

Commit ab51e35

Browse files
joehanbkendall
andauthored
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 <bkend@google.com>
1 parent dc02291 commit ab51e35

8 files changed

Lines changed: 169 additions & 644 deletions

File tree

npm-shrinkwrap.json

Lines changed: 4 additions & 606 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

package.json

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,6 @@
135135
"portfinder": "^1.0.32",
136136
"progress": "^2.0.3",
137137
"proxy-agent": "^6.3.0",
138-
"request": "^2.87.0",
139138
"retry": "^0.13.1",
140139
"rimraf": "^3.0.0",
141140
"semver": "^7.5.2",
@@ -189,7 +188,6 @@
189188
"@types/puppeteer": "^5.4.2",
190189
"@types/react": "^18.0.20",
191190
"@types/react-dom": "^18.0.6",
192-
"@types/request": "^2.48.1",
193191
"@types/retry": "^0.12.1",
194192
"@types/rimraf": "^2.0.3",
195193
"@types/semver": "^6.0.0",

scripts/storage-emulator-integration/utils.ts

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import * as fs from "fs";
22
import * as path from "path";
3-
import * as request from "request";
3+
import fetch from "node-fetch";
44
import * as crypto from "crypto";
55
import * as os from "os";
66
import { FrameworkOptions } from "../integration-helpers/framework";
@@ -81,11 +81,7 @@ export function writeToFile(filename: string, contents: Buffer, tmpDir: string):
8181
* Resets the storage layer of the Storage Emulator.
8282
*/
8383
export async function resetStorageEmulator(emulatorHost: string) {
84-
await new Promise<void>((resolve) => {
85-
request.post(`${emulatorHost}/internal/reset`, () => {
86-
resolve();
87-
});
88-
});
84+
await fetch(`${emulatorHost}/internal/reset`, { method: "POST" });
8985
}
9086

9187
export async function getProdAccessToken(serviceAccountKey: any): Promise<string> {

src/apiv2.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ const CLI_VERSION: string = pkg.version;
2020

2121
const GOOG_QUOTA_USER = "x-goog-quota-user";
2222

23-
export type HttpMethod = "GET" | "PUT" | "POST" | "DELETE" | "PATCH";
23+
export type HttpMethod = "GET" | "PUT" | "POST" | "DELETE" | "PATCH" | "OPTIONS" | "HEAD";
2424

2525
interface BaseRequestOptions<T> extends VerbOptions {
2626
method: HttpMethod;
@@ -184,6 +184,13 @@ export class Client {
184184
return this.request<unknown, ResT>(reqOptions);
185185
}
186186

187+
options<ResT>(path: string, options: ClientVerbOptions = {}): Promise<ClientResponse<ResT>> {
188+
const reqOptions: ClientRequestOptions<unknown> = Object.assign(options, {
189+
method: "OPTIONS",
190+
path,
191+
});
192+
return this.request<unknown, ResT>(reqOptions);
193+
}
187194
/**
188195
* Makes a request as specified by the options.
189196
* By default, this will:

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

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import fetch from "node-fetch";
1+
import fetch, { Response } from "node-fetch";
22
import * as fs from "fs";
33
import * as path from "path";
44
import * as yaml from "js-yaml";
@@ -71,8 +71,7 @@ export async function detectFromPort(
7171
runtime: runtimes.Runtime,
7272
timeout = 10_000 /* 10s to boot up */
7373
): Promise<build.Build> {
74-
// The result type of fetch isn't exported
75-
let res: { text(): Promise<string>; status: number };
74+
let res: Response;
7675
const timedOut = new Promise<never>((resolve, reject) => {
7776
setTimeout(() => {
7877
reject(new FirebaseError("User code failed to load. Cannot determine backend specification"));

src/functionsShellCommandAction.ts

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22
import * as clc from "colorette";
33
import * as repl from "repl";
44
import * as _ from "lodash";
5-
import * as request from "request";
65
import * as util from "util";
76

87
import * as shell from "./emulator/functionsEmulatorShell";
@@ -14,8 +13,9 @@ import { logger } from "./logger";
1413
import { EMULATORS_SUPPORTED_BY_FUNCTIONS, EmulatorInfo, Emulators } from "./emulator/types";
1514
import { EmulatorHubClient } from "./emulator/hubClient";
1615
import { resolveHostAndAssignPorts } from "./emulator/portUtils";
17-
import { Options } from "./options";
1816
import { Constants } from "./emulator/constants";
17+
import { Options } from "./options";
18+
import { HTTPS_SENTINEL } from "./localFunction";
1919
import { needProjectId } from "./projectUtils";
2020

2121
const serveFunctions = new FunctionsServer();
@@ -125,10 +125,8 @@ export const actionFunction = async (options: Options) => {
125125
);
126126

127127
const writer = (output: any) => {
128-
// Prevent full print out of Request object when a request is made
129-
// @ts-ignore
130-
if (output instanceof request.Request) {
131-
return "Sent request to function.";
128+
if (output === HTTPS_SENTINEL) {
129+
return HTTPS_SENTINEL;
132130
}
133131
return util.inspect(output);
134132
};

src/localFunction.ts

Lines changed: 148 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,14 @@
11
import * as uuid from "uuid";
2-
import * as request from "request";
32

43
import { encodeFirestoreValue } from "./firestore/encodeFirestoreValue";
54
import * as utils from "./utils";
65
import { EmulatedTriggerDefinition } from "./emulator/functionsEmulatorShared";
76
import { FunctionsEmulatorShell } from "./emulator/functionsEmulatorShell";
87
import { AuthMode, AuthType, EventOptions } from "./emulator/events/types";
8+
import { Client, ClientResponse, ClientVerbOptions } from "./apiv2";
9+
10+
// HTTPS_SENTINEL is sent when a HTTPS call is made via functions:shell.
11+
export const HTTPS_SENTINEL = "Request sent to function.";
912

1013
/**
1114
* LocalFunction produces EmulatedTriggerDefinition into a function that can be called inside the nodejs repl.
@@ -33,25 +36,119 @@ export default class LocalFunction {
3336
});
3437
}
3538

36-
private constructCallableFunc(
37-
data: string | object,
38-
opts: { instanceIdToken?: string }
39-
): request.Request {
39+
private constructCallableFunc(data: string | object, opts: { instanceIdToken?: string }): void {
4040
opts = opts || {};
4141

4242
const headers: Record<string, string> = {};
4343
if (opts.instanceIdToken) {
4444
headers["Firebase-Instance-ID-Token"] = opts.instanceIdToken;
4545
}
4646

47-
return request.post({
48-
callback: (...args) => this.requestCallBack(...args),
49-
baseUrl: this.url,
50-
uri: "",
51-
body: { data },
52-
json: true,
53-
headers: headers,
47+
if (!this.url) {
48+
throw new Error("No URL provided");
49+
}
50+
51+
const client = new Client({ urlPrefix: this.url, auth: false });
52+
void client
53+
.post<any, any>("", data, { headers })
54+
.then((res) => {
55+
this.requestCallBack<any>(undefined, res, res.body);
56+
})
57+
.catch((err) => {
58+
this.requestCallBack(err);
59+
});
60+
}
61+
62+
private constructHttpsFunc(): requestShim {
63+
if (!this.url) {
64+
throw new Error("No URL provided");
65+
}
66+
const callClient = new Client({ urlPrefix: this.url, auth: false });
67+
type verbFn = (...args: any) => Promise<string>;
68+
const verbFactory = (
69+
hasRequestBody: boolean,
70+
method: (
71+
path: string,
72+
bodyOrOpts?: any,
73+
opts?: ClientVerbOptions
74+
) => Promise<ClientResponse<any>>
75+
): verbFn => {
76+
return async (pathOrOptions?: string | HttpsOptions, options?: HttpsOptions) => {
77+
const { path, opts } = this.extractArgs(pathOrOptions, options);
78+
try {
79+
const res = hasRequestBody
80+
? await method(path, opts.body, toClientVerbOptions(opts))
81+
: await method(path, toClientVerbOptions(opts));
82+
this.requestCallBack(undefined, res, res.body);
83+
} catch (err) {
84+
this.requestCallBack(err);
85+
}
86+
return HTTPS_SENTINEL;
87+
};
88+
};
89+
90+
const shim = verbFactory(true, (path: string, json?: any, opts?: ClientVerbOptions) => {
91+
const req = Object.assign(opts || {}, {
92+
path: path,
93+
body: json,
94+
method: opts?.method || "GET",
95+
});
96+
return callClient.request(req);
5497
});
98+
const verbs: verbMethods = {
99+
post: verbFactory(true, (path: string, json?: any, opts?: ClientVerbOptions) =>
100+
callClient.post(path, json, opts)
101+
),
102+
put: verbFactory(true, (path: string, json?: any, opts?: ClientVerbOptions) =>
103+
callClient.put(path, json, opts)
104+
),
105+
patch: verbFactory(true, (path: string, json?: any, opts?: ClientVerbOptions) =>
106+
callClient.patch(path, json, opts)
107+
),
108+
get: verbFactory(false, (path: string, opts?: ClientVerbOptions) =>
109+
callClient.get(path, opts)
110+
),
111+
del: verbFactory(false, (path: string, opts?: ClientVerbOptions) =>
112+
callClient.delete(path, opts)
113+
),
114+
delete: verbFactory(false, (path: string, opts?: ClientVerbOptions) =>
115+
callClient.delete(path, opts)
116+
),
117+
options: verbFactory(false, (path: string, opts?: ClientVerbOptions) =>
118+
callClient.options(path, opts)
119+
),
120+
};
121+
return Object.assign(shim, verbs);
122+
}
123+
124+
private extractArgs(
125+
pathOrOptions?: string | HttpsOptions,
126+
options?: HttpsOptions
127+
): { path: string; opts: HttpsOptions } {
128+
// Case: No arguments provided
129+
if (!pathOrOptions && !options) {
130+
return { path: "/", opts: {} };
131+
}
132+
133+
// Case: pathOrOptions is provided as a string
134+
if (typeof pathOrOptions === "string") {
135+
return { path: pathOrOptions, opts: options || {} };
136+
}
137+
138+
// Case: pathOrOptions is an object (HttpsOptions), and options is not provided
139+
if (typeof pathOrOptions !== "string" && !!pathOrOptions && !options) {
140+
return { path: "/", opts: pathOrOptions };
141+
}
142+
143+
// Error case: Invalid combination of arguments
144+
if (typeof pathOrOptions !== "string" || !options) {
145+
throw new Error(
146+
`Invalid argument combination: Expected a string and/or HttpsOptions, got ${typeof pathOrOptions} and ${typeof options}`
147+
);
148+
}
149+
150+
// Default return, though this point should not be reached
151+
return { path: "/", opts: {} };
55152
}
56153

57154
constructAuth(auth?: EventOptions["auth"], authType?: AuthType): AuthMode {
@@ -120,11 +217,11 @@ export default class LocalFunction {
120217
};
121218
}
122219

123-
private requestCallBack(err: unknown, response: request.Response, body: string | object) {
220+
private requestCallBack<T>(err: unknown, response?: ClientResponse<T>, body?: string | object) {
124221
if (err) {
125222
return console.warn("\nERROR SENDING REQUEST: " + err);
126223
}
127-
const status = response ? response.statusCode + ", " : "";
224+
const status = response ? response.status + ", " : "";
128225

129226
// If the body is a string we want to check if we can parse it as JSON
130227
// and pretty-print it. We can't blindly stringify because stringifying
@@ -240,14 +337,46 @@ export default class LocalFunction {
240337
if (isCallable) {
241338
return (data: any, opt: any) => this.constructCallableFunc(data, opt);
242339
} else {
243-
return request.defaults({
244-
callback: (...args) => this.requestCallBack(...args),
245-
baseUrl: this.url,
246-
uri: "",
247-
});
340+
return this.constructHttpsFunc();
248341
}
249342
} else {
250343
return (data: any, opt: any) => this.triggerEvent(data, opt);
251344
}
252345
}
253346
}
347+
348+
// requestShim is a minimal implementation of the public API of the deprecated `request` package
349+
// We expose it as part of `functions:shell` so that we can keep the previous API while removing
350+
// our dependency on `request`.
351+
interface requestShim extends verbMethods {
352+
(...args: any): any;
353+
// TODO(taeold/blidd/joehan) What other methods do we need to add? form? json? multipart?
354+
}
355+
356+
interface verbMethods {
357+
get(...args: any): any;
358+
post(...args: any): any;
359+
put(...args: any): any;
360+
patch(...args: any): any;
361+
del(...args: any): any;
362+
delete(...args: any): any;
363+
options(...args: any): any;
364+
}
365+
366+
// HttpsOptions is a subset of request's CoreOptions
367+
// https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/request/index.d.ts#L107
368+
// We intentionally omit options that are likely useless for `functions:shell`
369+
type HttpsOptions = {
370+
method?: "GET" | "PUT" | "POST" | "DELETE" | "PATCH" | "OPTIONS" | "HEAD";
371+
headers?: Record<string, any>;
372+
body?: any;
373+
qs?: any;
374+
};
375+
376+
function toClientVerbOptions(opts: HttpsOptions): ClientVerbOptions {
377+
return {
378+
method: opts.method,
379+
headers: opts.headers,
380+
queryParams: opts.qs,
381+
};
382+
}

src/test/apiv2.spec.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,7 @@ describe("apiv2", () => {
158158
method: "GET",
159159
path: "/path/to/foo",
160160
});
161-
await expect(r).to.eventually.be.rejectedWith(FirebaseError, /Unable to parse JSON/);
161+
await expect(r).to.eventually.be.rejectedWith(FirebaseError);
162162
expect(nock.isDone()).to.be.true;
163163
});
164164

0 commit comments

Comments
 (0)