-
-
Notifications
You must be signed in to change notification settings - Fork 365
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 multipart boundary mismatch #540
Fix multipart boundary mismatch #540
Conversation
ead7dc5
to
8306b27
Compare
8306b27
to
1a20029
Compare
@sholladay you were correct in your comment when you questioned if all options were being exposed as instance properties. Please let me know what you think about this change. |
source/core/constants.ts
Outdated
@@ -58,3 +58,20 @@ export const kyOptionKeys: KyOptionsRegistry = { | |||
onDownloadProgress: true, | |||
fetch: true, | |||
}; | |||
|
|||
export const requestOptions = new Set([ |
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.
Is there any other way than having a hard-coded list here? We'll have to keep this up to date and if we miss something, it could cause problems.
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.
I really want to avoid this too. Let me give it some thought.
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.
Maybe a similar solution to the KyOptionsRegistry
can be employed here to defer the list keeping to Typescript and undici. I will try this.
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.
So there are two RequestInit interfaces that are relevant here: the one exported from node's undici-types
and the one exported from Typescript's lib.dom.d.ts
.
Common fields present in both interfaces:
method
keepalive
headers
body
redirect
integrity
signal
credentials
mode
referrer
referrerPolicy
window
Unique to undici-types
:
dispatcher
duplex
Unique to Typescript's lib.dom.d.ts
:
cache
Standard Request Options as specified in MDN
Present in both undici-types
and lib.dom.d.ts
:
method
headers
body
mode
credentials
redirect
referrer
referrerPolicy
integrity
keepalive
signal
Present only in lib.dom.d.ts
:
cache
Missing in both undici-types
and lib.dom.d.ts
:
priority
(⚠️ this field is still considered experimental but it is already supported in Chromium based browsers)
With all of this being said, I think we can leave priority
out and let it be detected as an unknown property since it will still be passed down to fetch.
Please let me know what you think of this approach @sindresorhus and @sholladay.
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.
Yeah, that sounds like a good approach.
…ons based on Typescript interfaces
a34f99b
to
9560921
Compare
Thanks for the quick fix 👍 |
Turns out that the read-only
body
property of the Request interface is not [yet] supported by Firefox which causes #539.Changes:
Fixes: #539 (introduced in #536)