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
Represent non-stringified JSON request body as an [object Object] string #881
Represent non-stringified JSON request body as an [object Object] string #881
Conversation
I'd like to ask for help with adding a test for this. Following the existing tests as examples, I've written this test: test('converts a non-stringified json body to [object Object] string', function() {
return fetch('/request', {
method: 'post',
headers: {
'content-type': 'application/json'
},
body: {stringified: false}
})
.then(function(response) {
return response.json()
})
.then(function(request) {
assert.equal(request.data, '[object Object]')
})
}) However, Line 594 in a8aa427
Assertions on The way I've verified this fix works is by running tests in my reproduction repository that asserts the arguments of the |
if (typeof body === 'string') { | ||
this.headers.set('content-type', 'text/plain;charset=UTF-8') | ||
} else if (this._bodyBlob && this._bodyBlob.type) { | ||
this.headers.set('content-type', this._bodyBlob.type) | ||
} else if (support.searchParams && URLSearchParams.prototype.isPrototypeOf(body)) { | ||
this.headers.set('content-type', 'application/x-www-form-urlencoded;charset=UTF-8') | ||
} | ||
} else if (contentType.includes('json') && typeof this._bodyInit !== '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.
This isn't a reliable JSON request body content-type header check. Would you recommend to provide application/json; charset=UTF-8
instead?
I'm thinking that this behavior should also affect any JSON-like content-type header (i.e. application/hal+json
).
thk sir |
@@ -246,14 +246,20 @@ function Body() { | |||
this._bodyText = body = Object.prototype.toString.call(body) |
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.
Alternatively, we can set this._bodyInit
to equal this._bodyText
in this closure. It fixes the issue, but the solution becomes less deterministic not being connected with the request's content-type. I'm not sure it would be a good idea to always reset the body to its textual counterpart, but I would appreciate some feedback.
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.
this._bodyText
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.
This is released in v3.6.0 -- https://github.com/github/fetch/releases/tag/v3.6.0 |
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
Ensures
request._bodyInit
(the internal representation of a request body) is always set to a[object Object]
string representation of a body in case it has a non-stringified JSON body and theContent-Type
header stating an expected JSON body.Why