mobile: node-js bindings for Envoy Client#43822
mobile: node-js bindings for Envoy Client#43822paul-r-gall wants to merge 2 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Paul Ogilby <pgal@google.com>
abeyad
left a comment
There was a problem hiding this comment.
Woohoo! I don't really know TS so have very little meaningful to add, but should we add a CI workflow as well? I'm fine if we do it in a follow up PR
Signed-off-by: Paul Ogilby <pgal@google.com>
| * node fetch_client.js <url> | ||
| */ | ||
|
|
||
| const { EnvoyClient, LogLevel } = require('../../../library/nodejs/index.js'); |
There was a problem hiding this comment.
Prefer building modern libraries with ESM ({type: "module"} in package.json) and use import instead of require.
There was a problem hiding this comment.
You shouldn't commit both .ts and .js to your git repo. Only .ts is typically committed, with .js being a build artifact.
| import * as path from 'path'; | ||
|
|
||
| // Load the native addon | ||
| const addonPath = path.join(__dirname, 'envoy_mobile_node.node'); |
There was a problem hiding this comment.
prefer import.meta.dirname to __dirname which is not compatible with ESM
| export enum LogLevel { | ||
| Trace = 0, | ||
| Debug = 1, | ||
| Info = 2, | ||
| Warn = 3, | ||
| Error = 4, | ||
| Critical = 5, | ||
| Off = 6, | ||
| } |
There was a problem hiding this comment.
I generally try to avoid using enums as they are not erasable syntax. Would prefer:
export const LogLevel {
Trace: 0,
Debug: 1,
Info: 2,
Warn: 3,
Error: 4,
Critical: 5,
Off: 6
} as const| private engine: any; | ||
| private streamClient: any; |
There was a problem hiding this comment.
Dangerous to have any typed objects. These should ideally have concrete types.
| @@ -0,0 +1,121 @@ | |||
| const path = require('path'); | |||
There was a problem hiding this comment.
This file is the same as index.ts but without types. Instead, you should leave this as a generated file from the .ts.
(Probably via rules_ts, though I haven't built typescript with bazel in OSS tbh)
| logLevel?: LogLevel; | ||
| } | ||
|
|
||
| export interface FetchOptions { |
There was a problem hiding this comment.
It looks like this is mostly already the case, but if you want to make the Envoy bindings for node.js ergonomic, the ECMAScript standard includes interfaces for the fetch API. I suggest following those interfaces:
fetch(), it's first parameter (or second, for the overloaded version), RequestInit and its return value, Response.
That way, using Envoy client from nodejs is a drop-in replacement for anyone using the fetch API, whether in Node.js via node-fetch, Deno/Bun native fetch, or in a browser (such as a future wasm port of envoy client).
A couple comments on specific changes to make below.
| body?: string | Buffer | Uint8Array; | ||
| } | ||
|
|
||
| export class EnvoyResponse { |
There was a problem hiding this comment.
Suggest adding the remaining nonnullable readonly properties.
The types from MDN: https://raw.githubusercontent.com/microsoft/TypeScript/refs/heads/main/src/lib/dom.generated.d.ts#:~:text=interface%20Response%20extends%20Body and https://raw.githubusercontent.com/microsoft/TypeScript/refs/heads/main/src/lib/dom.generated.d.ts#:~:text=eventInitDict%3A%20BlobEventInit)%3A%20BlobEvent%3B%0A%7D%3B-,interface%20Body,-%7B%0A%20%20%20%20/**%20%5BMDN%20Reference%5D(https
interface Body {
/** [MDN Reference](https://developer.mozilla.org/docs/Web/API/Request/body) */
readonly body: ReadableStream<Uint8Array<ArrayBuffer>> | null;
/** [MDN Reference](https://developer.mozilla.org/docs/Web/API/Request/bodyUsed) */
readonly bodyUsed: boolean;
/** [MDN Reference](https://developer.mozilla.org/docs/Web/API/Request/arrayBuffer) */
arrayBuffer(): Promise<ArrayBuffer>;
/** [MDN Reference](https://developer.mozilla.org/docs/Web/API/Request/blob) */
blob(): Promise<Blob>;
/** [MDN Reference](https://developer.mozilla.org/docs/Web/API/Request/bytes) */
bytes(): Promise<Uint8Array<ArrayBuffer>>;
/** [MDN Reference](https://developer.mozilla.org/docs/Web/API/Request/formData) */
formData(): Promise<FormData>;
/** [MDN Reference](https://developer.mozilla.org/docs/Web/API/Request/json) */
json(): Promise<any>;
/** [MDN Reference](https://developer.mozilla.org/docs/Web/API/Request/text) */
text(): Promise<string>;
}
/**
* The **`Response`** interface of the Fetch API represents the response to a request.
*
* [MDN Reference](https://developer.mozilla.org/docs/Web/API/Response)
*/
interface Response extends Body {
/**
* The **`headers`** read-only property of the Response interface contains the Headers object associated with the response.
*
* [MDN Reference](https://developer.mozilla.org/docs/Web/API/Response/headers)
*/
readonly headers: Headers;
/**
* The **`ok`** read-only property of the Response interface contains a Boolean stating whether the response was successful (status in the range 200-299) or not.
*
* [MDN Reference](https://developer.mozilla.org/docs/Web/API/Response/ok)
*/
readonly ok: boolean;
/**
* The **`redirected`** read-only property of the Response interface indicates whether or not the response is the result of a request you made which was redirected.
*
* [MDN Reference](https://developer.mozilla.org/docs/Web/API/Response/redirected)
*/
readonly redirected: boolean;
/**
* The **`status`** read-only property of the Response interface contains the HTTP status codes of the response.
*
* [MDN Reference](https://developer.mozilla.org/docs/Web/API/Response/status)
*/
readonly status: number;
/**
* The **`statusText`** read-only property of the Response interface contains the status message corresponding to the HTTP status code in Response.status.
*
* [MDN Reference](https://developer.mozilla.org/docs/Web/API/Response/statusText)
*/
readonly statusText: string;
/**
* The **`type`** read-only property of the Response interface contains the type of the response. The type determines whether scripts are able to access the response body and headers.
*
* [MDN Reference](https://developer.mozilla.org/docs/Web/API/Response/type)
*/
readonly type: ResponseType;
/**
* The **`url`** read-only property of the Response interface contains the URL of the response. The value of the url property will be the final URL obtained after any redirects.
*
* [MDN Reference](https://developer.mozilla.org/docs/Web/API/Response/url)
*/
readonly url: string;
/**
* The **`clone()`** method of the Response interface creates a clone of a response object, identical in every way, but stored in a different variable.
*
* [MDN Reference](https://developer.mozilla.org/docs/Web/API/Response/clone)
*/
clone(): Response;
}
| resolve(new EnvoyResponse(responseStatus, responseHeaders, Buffer.concat(bodyChunks))); | ||
| } | ||
| }, | ||
| onData: (data: Buffer, endStream: boolean) => { |
There was a problem hiding this comment.
Does the Envoy C++ code intend to buffer the response body?
If so, you could, if you like, avoid reading it into the JS heap until/unless asked for via one of the promise-returning methods in the response object.
| } else if (Buffer.isBuffer(options.body)) { | ||
| bodyBuffer = options.body; | ||
| } else { | ||
| bodyBuffer = Buffer.from(options.body as Uint8Array); |
There was a problem hiding this comment.
I don't think this is safe... I'd use else if (options.body instanceOf Uint8Array) {} else {throw Error()}
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
Commit Message: node-js bindings for Envoy Client
Risk Level:
Testing: Examples plus integration test.