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

improve HeadersInit type #2373

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

improve HeadersInit type #2373

wants to merge 1 commit into from

Conversation

Ethan-Arrowood
Copy link
Collaborator

This PR improves the HeadersInit type. The spec sorta gets this wrong (https://fetch.spec.whatwg.org/#typedefdef-headersinit) It only specifies record<ByteString, ByteString>, but record<ByteString, ByteString | undefined also works. I've tested it in multiple browsers and our implementation, and it works fine.

This allows me to pass an IncomingHTTPHeaders object directly to the constructor (which is valid for all other reasons except for this missing | undefined).

@codecov-commenter
Copy link

codecov-commenter commented Oct 24, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (e39a632) 85.54% compared to head (0a053f7) 85.84%.
Report is 65 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2373      +/-   ##
==========================================
+ Coverage   85.54%   85.84%   +0.29%     
==========================================
  Files          76       76              
  Lines        6858     6886      +28     
==========================================
+ Hits         5867     5911      +44     
+ Misses        991      975      -16     
Files Coverage Δ
index-fetch.js 100.00% <100.00%> (ø)
lib/api/api-stream.js 100.00% <100.00%> (ø)
lib/api/readable.js 91.53% <100.00%> (ø)
lib/core/connect.js 80.00% <100.00%> (-1.25%) ⬇️
lib/core/request.js 92.38% <100.00%> (+5.29%) ⬆️
lib/fetch/body.js 97.68% <100.00%> (ø)
lib/fetch/global.js 100.00% <ø> (ø)
lib/fetch/index.js 86.39% <100.00%> (+1.19%) ⬆️
lib/pool.js 100.00% <100.00%> (ø)
lib/websocket/connection.js 89.15% <100.00%> (+0.13%) ⬆️
... and 3 more

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@KhafraDev KhafraDev left a comment

Choose a reason for hiding this comment

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

Technically webidl will handle any type we pass (null, numbers, objects, arrays, etc.) but we should be explicit in what types/values are expected (being what the spec tells us). Typescript's own DOM types do as much https://github.com/microsoft/TypeScript/blob/11a04ed9086e1e89cab970901b3496d5a67bcf59/src/lib/dom.generated.d.ts#L27983

@KhafraDev
Copy link
Member

IMO the ReadonlyArray should be removed because it's not an expected value, I'm really not sure how that ended up there. Webidl will handle it, but probably not in an expected way, and we probably shouldn't encourage people to misuse it.

@Ethan-Arrowood
Copy link
Collaborator Author

I hear ya but we should match browser behavior. And honestly our types should match our implementation. If the spec does say to fail on undefined, and if our implementation doesn't fail on it either, I see little harm in adjusting the types so that we can avoid an unnecessary @ts-expect-error

@KhafraDev
Copy link
Member

If you pass undefined as the value it'll get serialized to "undefined". It'll only lead to unexpected behavior. Plus the type, if we're following webidl, would be something similar to [any, any][] | Headers | Record<any, any> which is less explicit and less useful than stricter typings.

kodiakhq bot pushed a commit to vercel/vercel that referenced this pull request Nov 8, 2023
…r.mts` (#10767)

In a recent undici update, setting the `host` header for fetch requests became invalid (nodejs/undici#2322). 

We relied on this in order to proxy serverless dev server requests via `@vercel/node`. 

This PR replaces the usage of `undici.fetch` with `undici.request`. 

It is blocked by an `undici` type change: nodejs/undici#2373
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants