Image.create sends base64 string with application/octet-stream #1320
Conversation
panteliselef
requested review from
SokratisVidros and
nikosdouvlis
as code owners
🦋 Changeset detectedLatest commit: d1536eb The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
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 |
panteliselef
changed the title
![jit-ci[bot]](https://avatars.githubusercontent.com/in/142441?s=60&v=4){kind=small}
| if (typeof body['file'] === 'string') { | ||
| fd = body['file']; | ||
| headers = new Headers(); | ||
| headers.set('Content-Type', 'application/octet-stream'); |
There was a problem hiding this comment.
@panteliselef I would expect this to happen in fapiClient thought an option of something
There was a problem hiding this comment.
I think this is the correct approach. This will reach the fapiClient eventually and the Content-Type header won't be reset there.
There was a problem hiding this comment.
Nicely done @panteliselef!
I'm not approving since I think we should change the logic a bit in the fapiClient. Hope it's not just me who feels it's safer to split the concerns.
Can we also add some tests for this change? One for Image.create and one for fapiClient that the Content-Type header will be respected and the body not tampered with. Unless such a test already exists of course, didn't check. 😊
| @@ -179,7 +179,7 @@ export default function createFapiClient(clerkInstance: Clerk): FapiClient { | |||
|
|
|||
| // In case FormData is provided, we don't want to mess with the headers, | |||
| // because for file uploads the header is properly set by the browser. | |||
| if (method !== 'GET' && !(body instanceof FormData)) { | |||
| if (method !== 'GET' && !(body instanceof FormData) && typeof body !== 'string') { | |||
There was a problem hiding this comment.
🔧 I think we're confusing two different concerns here and maybe it's a good time to separate them.
I believe this part of the code wants to do the following:
- Default to application/x-www-form-urlencoded for all non-GET requests, unless there's reason not to.
- Make the request body a query string, so that it's suitable to be sent as form urlencoded values to the server.
The additional check for body not being an instance of FormData was added by me and it's a hack. What I really wanted to do is set the Content-Type to multipart/form. Unfortunately, when sending multipart data, you have to specify the body parts in the content-type header. It's not as simple as setting the content-type to the "multipart/form" value. I chose to leave the burden of constructing the proper header to the native API instead. It's certain that it will be constructed correctly.
I think we should rewrite part of this method to do something like
// Initialize the headers if they're not provided.
if (!requestInit.headers) {
requestInit.headers = new Headers();
}
// Set the default content type for non-GET requests.
// Skip for FormData, because the browser knows how to construct it later on.
// Skip if the content-type header has already been set, somebody intends to override it.
if (method !== 'GET' && !(body instanceof FormData) && !requestInit.headers.has('content-type')) {
requestInit.headers.set('content-type', 'application/x-www-form-urlencoded');
}
// Massage the body depending on the content type if needed.
// Currently, this is needed only for form-urlencoded, so that the values reach the server in the form
// foo=bar&baz=bar&whatever=1
if (String(requestInit.headers.get('content-type')) === 'application/x-www-form-urlencoded') {
requestInit.body = qs.stringify(body, { encoder: camelToSnakeEncoder, indices: false });
}A quick search in the clerkJS package reveals that we're only setting the content-type here, so I think the change is safe, we won't break anything.
| if (typeof body['file'] === 'string') { | ||
| fd = body['file']; | ||
| headers = new Headers(); | ||
| headers.set('Content-Type', 'application/octet-stream'); |
There was a problem hiding this comment.
I think this is the correct approach. This will reach the fapiClient eventually and the Content-Type header won't be reset there.
|
!snapshot |
|
Hey @panteliselef - the snapshot version command generated the following package versions:
Tip: use the snippet copy button below to quickly install the required packages. # @clerk/backend
npm i @clerk/backend@0.22.0-snap.a0dd011# @clerk/chrome-extension
npm i @clerk/chrome-extension@0.3.8-snap.a0dd011# @clerk/clerk-js
npm i @clerk/clerk-js@4.45.1-snap.a0dd011# eslint-config-custom
npm i eslint-config-custom@0.3.0# @clerk/clerk-expo
npm i @clerk/clerk-expo@0.17.8-snap.a0dd011# @clerk/fastify
npm i @clerk/fastify@0.4.6-snap.a0dd011# gatsby-plugin-clerk
npm i gatsby-plugin-clerk@4.3.7-snap.a0dd011# @clerk/localizations
npm i @clerk/localizations@1.17.1-snap.a0dd011# @clerk/nextjs
npm i @clerk/nextjs@4.20.0-snap.a0dd011# @clerk/clerk-react
npm i @clerk/clerk-react@4.19.0-snap.a0dd011# @clerk/remix
npm i @clerk/remix@2.6.6-snap.a0dd011# @clerk/clerk-sdk-node
npm i @clerk/clerk-sdk-node@4.10.4-snap.a0dd011# @clerk/shared
npm i @clerk/shared@0.18.0-snap.a0dd011# @clerk/themes
npm i @clerk/themes@1.7.5# @clerk/types
npm i @clerk/types@3.41.1-snap.a0dd011 |
Sets Content type to be `application/octet-stream`
|
This PR has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Type of change
Packages affected
@clerk/clerk-js@clerk/clerk-react@clerk/nextjs@clerk/remix@clerk/types@clerk/themes@clerk/localizations@clerk/clerk-expo@clerk/backend@clerk/clerk-sdk-node@clerk/shared@clerk/fastify@clerk/chrome-extensiongatsby-plugin-clerkbuild/tooling/choreDescription
npm testruns as expected.npm run buildruns as expected.