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] Default body parsing to binary #1890

Merged
merged 3 commits into from
Jul 13, 2021
Merged

[fix] Default body parsing to binary #1890

merged 3 commits into from
Jul 13, 2021

Conversation

benmccann
Copy link
Member

@benmccann benmccann commented Jul 12, 2021

In response to @drzax's comment (https://github.com/sveltejs/kit/pull/1687/files#r667643869) it occurs to me that the way we're treating body content type is a bit backwards. We should treat is as bytes unless it's a known type. The user can always convert bytes into whatever they're expecting. But if we try to treat it as text unless we detect a known content-type, we're doomed to failure since it would be hard to create a super exhaustive list of mime types to ensure we're handling them all correctly

(This is PR aligns with how Play Framework works, which is one of the web frameworks I'm most familiar with. It also aligns with how our own parse_body works)

@changeset-bot
Copy link

changeset-bot bot commented Jul 12, 2021

🦋 Changeset detected

Latest commit: 0d11fe4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@sveltejs/adapter-cloudflare-workers Patch
@sveltejs/adapter-netlify Patch
@sveltejs/kit Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@JeanJPNM
Copy link
Contributor

JeanJPNM commented Jul 12, 2021

This will prevent it from failing if the header has directives attached to it

	if (!content_type) return true; // defaults to json
	const [type] = content_type.split(';'); // get the mime type
	return (
		type === 'text/plain' ||
		type === 'application/json' ||
		type === 'application/x-www-form-urlencoded' ||
		type === 'multipart/form-data'
	);

@benmccann
Copy link
Member Author

Thanks for taking a look @JeanJPNM ! Both good suggestions!

@benmccann benmccann merged commit 4720b67 into master Jul 13, 2021
@benmccann benmccann deleted the content-type branch July 13, 2021 04:37
@utkarshkukreti
Copy link
Contributor

@benmccann, with this change, an endpoint in my app which returns application/xml broke.

export const get = () => {
    const body = '<foo />';
    return { status: 200, headers: { 'content-type': 'application/xml' }, body }
};
Invalid response from route /foo.xml: body must be an instance of Uint8Array if content-type is not a supported textual content-type

Should application/xml be added to the list of textual content types? or am I missing something and there's a better way?

@JeanJPNM
Copy link
Contributor

JeanJPNM commented Jul 13, 2021

Well, currently you can use a workaround:

export const get = () => {
  const body = "<foo />";
  return {
    status: 200,
    headers: { "content-type": "application/xml" },
    body: new TextEncoder().encode(body),
  };
};

@benmccann
Copy link
Member Author

I sent a fix here: #1900

@utkarshkukreti
Copy link
Contributor

@JeanJPNM @benmccann thanks!

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