Usage of _json #296

Closed
Marsup opened this Issue Aug 6, 2012 · 5 comments

Comments

Projects
None yet
4 participants
Contributor

Marsup commented Aug 6, 2012

Hello Mikeal,

I have been using request for quite a while now to test my APIs with defaults({ json: true }) which is convenient for a json API.

With the arrival of the new express/connect, it is less tolerant on the input so it fails on all queries with an empty body.
I have switched to using defaults({ _json: true }) which works very well for my case but since it's undocumented, I was wondering if it should be part of the official API under a nicer name or not

What do you think ?

Would like to know this too. Thanks to this issue I was able to find the _json parameter which is what I needed since I did NOT want the request content-type set to application/json but did want the response parsed as json. For a REST API it's quite common to post data using a form content-type and then expect a json response - but using json: true doesn't seem to allow for this. The ability to specify _json: true seems to work around this issue but making it a mainstream feature and documenting it would be good.

Owner

mikeal commented Sep 19, 2012

i don't understand why a change in express is effecting request. it's a bad idea to us _ properties, they are internal.

Contributor

Marsup commented Sep 20, 2012

I know _ properties are internals, that's why I asked for it to go mainstream because I don't like doing it either.

If you open a request with Content-Type: application/json, connect's bodyParser wants a body and will fail otherwise.

Here is a minimal gist to show the error.
With the previous version you could expect a response, with the latest you get Error: invalid json.
The code responsible for this is here.

tellnes commented Sep 23, 2012

+1

Owner

mikeal commented Sep 25, 2012

we should only send content-type if we send a body. that's a bug, we should just fix it.

mikeal closed this in 4797f88 Oct 21, 2012

@mikeal mikeal added a commit that referenced this issue Oct 21, 2012

@mikeal mikeal Merge pull request #332 from Marsup/issue-296
Fix #296 - Only set Content-Type if body exists
59cf5e6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment