-
Notifications
You must be signed in to change notification settings - Fork 570
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
fix(fetch): improve Headers and Request type-compatibility #1947
Conversation
@@ -68,7 +68,7 @@ export declare class Headers implements SpecIterable<[string, string]> { | |||
readonly keys: () => SpecIterableIterator<string> | |||
readonly values: () => SpecIterableIterator<string> | |||
readonly entries: () => SpecIterableIterator<[string, string]> | |||
readonly [Symbol.iterator]: () => SpecIterator<[string, string]> | |||
readonly [Symbol.iterator]: () => SpecIterableIterator<[string, string]> |
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.
It appears that the iterator symbol on Headers
must be iterable.
@@ -146,7 +146,8 @@ export declare class Request implements BodyMixin { | |||
readonly method: string | |||
readonly mode: RequestMode | |||
readonly redirect: RequestRedirect | |||
readonly referrerPolicy: string | |||
readonly referrer: string | |||
readonly referrerPolicy: ReferrerPolicy |
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.
Also narrowing down the referrer policy as we already have the ReferrerPolicy
type used for RequestInit
. It cannot really be anything else, to my best knowledge.
@@ -146,7 +146,8 @@ export declare class Request implements BodyMixin { | |||
readonly method: string | |||
readonly mode: RequestMode | |||
readonly redirect: RequestRedirect | |||
readonly referrerPolicy: string | |||
readonly referrer: string |
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.
Adding the missing referrer
property.
With these changes, I can eliminate most of the type-related errors in my usage project. However, there seems to be an incongruence between the There are a few waves of errors that happen: 1.
|
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.
changes look fine, the body type fix can wait for another PR (although ReadableStream<Uint8Array> is correct)
By the way, I don't mind lending a hand with type testing in this repo. I've worked some time with it and it has proven rather useful. You can see an example of it in here. Basically, with a bit of a configuration, we can automate the type-checking of Undici examples against different versions of TypeScript even, and use those as tests to catch type-related regressions. I think in the light that Undici is not authored in TypeScript, type tests stand twice as useful. |
Any help is appreciated! Go for it. Can this PR land? It's still marked as draft. |
Hey, @mcollina. It's ready at its current state. Is it okay that some tests are failing on CI? They also fail for me locally (on |
I think there was a bug in one of the deps that was fixed. |
Could you add a test for the types? We use tsd. |
I'm a bit overwhelmed with work, I will add the test this/next week. |
I love GitHub. Let's put two buttons with totally opposite actions 3px away from each other. Sorry, will re-open. |
I'm using a simple reproduction example to test type congruence of Undici against
lib/dom
: