-
Notifications
You must be signed in to change notification settings - Fork 27
Removing lib.DOM dependency #405
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
base: v4.x
Are you sure you want to change the base?
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| * This is a local definition to avoid dependency on lib.dom. | ||
| * Compatible with Node.js native Fetch API body types. | ||
| */ | ||
| export type HttpResponseBodyInit = ReadableStream | Blob | ArrayBufferView | ArrayBuffer | string | null | undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential Issues with Type Definitions
Thank you for this PR! Removing the lib.DOM dependency is the right approach. However, I noticed a few potential compatibility gaps in the new type definitions:
1. Missing FormData and URLSearchParams in HttpResponseBodyInit
The standard BodyInit type (from Web/Node.js Fetch API) includes:
type BodyInit = ReadableStream | Blob | ArrayBufferView | ArrayBuffer | FormData | URLSearchParams | stringThe PR's definition:
type HttpResponseBodyInit = ReadableStream | Blob | ArrayBufferView | ArrayBuffer | string | null | undefinedIssue: FormData and URLSearchParams are missing. Users who pass these types as the response body will encounter type errors, even though it works at runtime.
2. HttpHeadersInit Missing Iterable Support
The standard HeadersInit type:
type HeadersInit = Headers | string[][] | Record<string, string> | Iterable<[string, string]>The PR's definition:
type HttpHeadersInit = Headers | Record<string, string> | [string, string][]Issue: Iterable<[string, string]> is not supported, so passing a Map<string, string> or other custom iterables will fail type checking.
3. Type Cast in HttpResponse.ts
this.#nativeRes = new Response(init.body as BodyInit, resInit);This cast bypasses the type system. If HttpResponseBodyInit is narrower than what Node.js's Response actually accepts, there could be inconsistencies where valid runtime values fail type checking.
Suggested Fix
export type HttpResponseBodyInit =
| ReadableStream
| Blob
| ArrayBufferView
| ArrayBuffer
| FormData
| URLSearchParams
| string
| null
| undefined;
export type HttpHeadersInit =
| Headers
| Record<string, string>
| Iterable<[string, string]>;Note:
FormDataandURLSearchParamsare globally available in Node.js 18+, which aligns with the minimum supported version for Azure Functions Node.js v4 programming model.
4. Test Coverage
Consider adding tests for FormData and URLSearchParams body types to ensure full coverage.
Overall, this is a great improvement! Just these minor additions would ensure full backward compatibility with the original BodyInit and HeadersInit types.
Removing the lib.DOM Dependency
Issue: #404