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

Fix multipart boundary mismatch #540

Merged
merged 3 commits into from
Oct 29, 2023

Conversation

kdelmonte
Copy link
Contributor

@kdelmonte kdelmonte commented Oct 27, 2023

Turns out that the read-only body property of the Request interface is not [yet] supported by Firefox which causes #539.

Changes:

  • Add functionality to run same test in multiple browsers including Firefox (⚠️ slows down CI by about 1 minute; ~15% slower)
  • Check Request constructor options explicitly.

Fixes: #539 (introduced in #536)

@kdelmonte kdelmonte force-pushed the multipart-boundary-mismatch branch 2 times, most recently from ead7dc5 to 8306b27 Compare October 27, 2023 08:38
@kdelmonte kdelmonte force-pushed the multipart-boundary-mismatch branch from 8306b27 to 1a20029 Compare October 27, 2023 08:40
@kdelmonte kdelmonte changed the title [WIP] Fix Multipart boundary mismatch Fix Multipart boundary mismatch Oct 27, 2023
@kdelmonte kdelmonte marked this pull request as ready for review October 27, 2023 09:29
@kdelmonte
Copy link
Contributor Author

@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.

@@ -58,3 +58,20 @@ export const kyOptionKeys: KyOptionsRegistry = {
onDownloadProgress: true,
fetch: true,
};

export const requestOptions = new Set([
Copy link
Owner

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Owner

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.

@kdelmonte kdelmonte force-pushed the multipart-boundary-mismatch branch from a34f99b to 9560921 Compare October 27, 2023 21:14
@kdelmonte kdelmonte changed the title Fix Multipart boundary mismatch Fix multipart boundary mismatch Oct 27, 2023
@sindresorhus sindresorhus merged commit 0e9d7bb into sindresorhus:main Oct 29, 2023
1 check passed
@sindresorhus
Copy link
Owner

Thanks for the quick fix 👍

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.

1.1.1 breaks multipart form boundary
2 participants