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

JSON option when posting data #174

Closed
paglias opened this issue Feb 19, 2016 · 43 comments
Closed

JSON option when posting data #174

paglias opened this issue Feb 19, 2016 · 43 comments
Labels
enhancement This change will extend Got features ✭ help wanted ✭

Comments

@paglias
Copy link

paglias commented Feb 19, 2016

Would it be possible to use the json: true option when posting data to set Content-Type to application/json and stringify the body with JSON.stringify?

@floatdrop
Copy link
Contributor

@paglias it is possible, but json option is responsible only for toggle parsing response from server and it will be kinda major change to make it stringify the body for request also.

@spencerhakim
Copy link

It would be a major change, but not necessarily a bad one. At least having the option to enable it would be nice. Like, if json is an object instead of just true seems like a reasonable compromise.

Edit - Tagging @sindresorhus, since it's his project

@sindresorhus
Copy link
Owner

@spencerhakim Can you provide a code example of how it would look like?

@sindresorhus
Copy link
Owner

Reopening, but we're not likely to do another major in the near future.

@sindresorhus sindresorhus reopened this Mar 28, 2016
@sindresorhus sindresorhus added the enhancement This change will extend Got features label Mar 28, 2016
@spencerhakim
Copy link

Current way:

var options = {
  json: true, // if truthy, parse *response* as JSON
  headers: {
    'Content-type': 'application/json'
  },
  body: JSON.stringify({ // have to manually stringify object in body
    name: 'whatever',
    category: 'toy',
    description: 'Hello World'
  })
};

got.post(`http://example.com/api/createThing`, options)
  .then((resp) => {
    // ...
  });

Possible new way (should be functionally equivalent to the above):

var options = {
  json: { // typeof json === 'object', not 'boolean', so send body as JSON instead of form-encoded
    name: 'whatever',
    category: 'toy',
    description: 'Hello World'
  }
};

got.post(`http://example.com/api/createThing`, options)
  .then((resp) => {
    // ...
  });

Coming from request, I was surprised that got didn't follow the same semantics for handling JSON or offer any way of automatically serializing sent objects as JSON. This approach should be backwards compatible with current usage, so I don't think it requires a major version bump.

@sindresorhus
Copy link
Owner

Wouldn't it make more sense to pass the object literal in body?

@spencerhakim
Copy link

Yes, but that approach would likely require a major version bump.

@bishtawi
Copy link

bishtawi commented Jun 4, 2016

@sindresorhus What about making the json attribute an object that takes req and res as keys. Setting req to true means the request body should be JSON encoded, setting res to true means the response body should be JSON parsed.

The current json attribute being a boolean could be deprecated but not removed till the next major rev bump. Setting json to true would be translated to { req: false, res: true}. Omitting the json attribute or setting it to false would translate to { req: false, res: false}. This would keep backwards compatibility while easily supporting a way to JSON encode the request body.

var options = {
  json: {
    req: true,
    res: true
  },
  body: {
    name: 'whatever',
    category: 'toy',
    description: 'Hello World'
  }
};

The above example would JSON encode/decode the request/response body.

@sindresorhus
Copy link
Owner

sindresorhus commented Jun 18, 2016

Not interested adding a temporary workaround when it's possible for you to stringify it yourself. We'll add support for passing object literal in body when json: true in the next major release.

@sindresorhus sindresorhus added this to the 7.0.0 milestone Jun 18, 2016
@floatdrop
Copy link
Contributor

@sindresorhus now passing object literal in body work as serializing it as application/x-www-form-urlencoded – wouldn't it make json option a bit confusing?

@sindresorhus
Copy link
Owner

@floatdrop I don't really see the confusion. json: true makes it clear it has different behavior. Just need clear docs.

request supports the same:

body - entity body for PATCH, POST and PUT requests. Must be a Buffer, String or ReadStream. If json is true, then body must be a JSON-serializable object.

@floatdrop
Copy link
Contributor

@sindresorhus it is clear, that json: true will change behaviour, but not in obvious way – without it Object in body was serialized as form, but with json: true it morphing to JSON object, which is not expected in many usecases (for example API that accepts form in body, but returns JSON).

Maybe it is better way to choose serialization based on content-type header?

@sindresorhus
Copy link
Owner

for example API that accepts form in body, but returns JSON

Hm, I didn't think of that use-case.

request has a separate form option for posting an object serialized as application/x-www-form-urlencoded. I think that would be a less surprising way. Since the JSON thing would be a breaking change, it might worth doing this too.

@bisubus
Copy link
Contributor

bisubus commented Jul 13, 2016

@sindresorhus The flexibility seems to be appropriate in this case. Switched to got from superagent recently. The latter has got 3 different methods (type, accept, parse) for setting serialization type, accept header and forced parsing.

@MarkHerhold
Copy link
Contributor

I just converted from request-promise and had the same need as others here. request-promise would automatically stringify JSON bodies and set the appropriate header. I solved this by wrapping got with a local module and doing the same. Here's a snippet from my wrapper:

Stringify JSON Objects and Arrays and set Content-Type

    // stringify object and array bodies
    if (options.json === true && options.body && (typeof options.body === 'object' || Array.isArray(options.body))) {
        options.headers = options.headers || {};
        options.headers['Content-Type'] = 'application/json';
        options.body = JSON.stringify(options.body);
    }

@alextes
Copy link
Contributor

alextes commented Sep 7, 2016

Since this is a very common use case in my experience and the json flag works very differently from request/request which is the dominant http / request lib. Similarly visionmedia/superagent doesn't require the coder do much thinking with regards to JSON posts.

I think many coders will require the same 5 - 10 min of messing about before they realise you do it manually with got. Perhaps even running into an unexpected error.

Adding documentation for this common use case makes a lot of sense to me. I think the cost is low, even if only considering the benefit to the maintainers not having to deal with people creating issues like this. Would a PR be welcome?

Unrelated but the same question for adding an index to the documentation. PR welcome? ^^

Thanks for creating what will undoubtedly be my new http lib goto.

@MarkHerhold
Copy link
Contributor

MarkHerhold commented Sep 7, 2016

Alex, I think you are on point. I too wrote a wrapper around got for
serializing JSON bodies.

@sindresorhus
Copy link
Owner

PR welcome for the change described in #174 (comment). Would be nice to have this included in the next major version.

@alextes
Copy link
Contributor

alextes commented Nov 23, 2016

After giving the code a thorough read (so concise<3) I can say I would love to take a stab at it @sindresorhus. I also feel I might be out of my depth with all the streaming going on. I'm hoping the greatest cats in nodejs won't mind that.

Maybe good to summarize.

  • add to current json flag effect / body behaviour.
    • If body is a plain object, it will be stringified with querystring.stringify and sent as application/x-www-form-urlencoded.
    • If json is true, and body is a plain object, it will be stringified with JSON.stringify and sent as application/json.

This poses a problem for people wanting to send a querystring but receive json and have it parsed. Which is to be solved as follows.

  • add form option
    • type: object(maybe string)
    • takes a plain object which will be stringified with querystring.stringify and sent as application/x-www-form-urlencoded.

This does leave the hairy situation where a user sets both json and form and then passes a plain object or string as the body. Since content types are mutually exclusive (I think) what happens?
This is assuming the default body behaviour is to stay the same.

Maybe you're going on the assumption querystring and json would both require explicit setting to affect the request. In that case, what is to be the default behaviour of body? Pass to req.end? What happens when passing an object which node doesn't support?

Final question, what about people wanting to send json but not receive it? Many will use json flag as they did with other libs, and perhaps be surprised when an error is thrown because we tried to parse their plain text response. These are probably few and far between. Still, asking the user to set a header and stringify the body themselves because you want to control your parsing on receiving does not seem straightforward.

Again, if it isn't too much trouble answering, I would love to take a stab at this PR ^^

@MarkHerhold
Copy link
Contributor

There are a lot of valid what-if questions here. My fear is that we implement this or another approach, someone ends up not liking it, then we add another set of options to accommodate that use case, and soon the got becomes 500k lines long, competing in the got vs request bloat competition. 😆

Alternatively, we could devise a plugin architecture, possibly via beforeRequest()/afterRequest() hooks, making this PR a plugin that ships with got, but not enabled by default. This would allow others to write their own JSON plugin that fits other use cases without bloating the core.

@alextes
Copy link
Contributor

alextes commented Nov 25, 2016

Hm. I think considering to generalize this case makes sense @MarkHerhold. I've also got to add that's a bigger PR than I think I can take on, and wouldn't without the cats weighing in, and they seem busy. I think I'll simply open a PR, making the most straightforward choices I can. I like the general idea of having optional pre / post request processing. I wouldn't know the best way to implement it.

@alextes
Copy link
Contributor

alextes commented Dec 12, 2016

I thought about this quite a bit when making my first attempt at implementing and now am pretty convinced it's a bad idea. I think having the 'shortcut props' makes more sense. So form, lets you easily send application/x-www-form-urlencoded, maybe formData for multipart/form-data, JSON for application/json. That leaves parsing freed up to be a wholly separate thing. Creating a clear distinction between serializing and deserializing which we shouldn't mix I'd say. Most of the inspiration for this thinking comes from looking around, mostly at superagent. A small bonus is that you can start treating deserializing manually by specifying a parsing function or you can start looking at the content-type in the response headers for default behavior parsing which would make a lot of sense too.

@sindresorhus
Copy link
Owner

I feel we're overthinking it a bit here and trying to consider too many imaginary edge-cases. The JSON option is just meant to cover the common case. If someone wants more advanced configurability they can just not use it.

Proposal:

  • If json: false, body will be required to be a string, buffer, readableStream.
  • If json: true, body will be required to be a plain object and will be JSON stringified and correct headers set and response will be JSON parsed.
  • If form: true, body will be required to be a plain object and will be stringified with querystring.stringify and sent as application/x-www-form-urlencoded. If json: true, only the response will be parsed.

I'm open to suggestions on how we could simplify this further.

@alextes
Copy link
Contributor

alextes commented May 3, 2017

Fair enough Sindre.
I'll PR the above. It would make any switchover to got a lot smoother.
Just to be clear, your proposal means dropping the default querystring serializing of object values passed as the body, like I proposed in #265 right?

@alextes
Copy link
Contributor

alextes commented May 4, 2017

I want to suggest we support all inputs that querystring.stringify and JSON.stringify support, or maybe their shared subset.

@sindresorhus
Copy link
Owner

your proposal means dropping the default querystring serializing of object values passed as the body, like I proposed in #265 right?

👍

I want to suggest we support all inputs that querystring.stringify and JSON.stringify support, or maybe their shared subset.

I'm not sure what this means. Can you elaborate?

@alextes
Copy link
Contributor

alextes commented May 5, 2017

I'm not sure what this means. Can you elaborate?

I like when things work the way I imagine them to work. When I use a request library, specify a body, and ask to stringify the body, I imagine it:

  • accepts any input the underlying stringify fn accepts
  • (bonus) is helpful and refuses to stringify something that might be accepted by the stringify fn but isn't what I meant to do.

Current proposal: only accept a plain object.
querystring.stringify accepts everything:

> qstr('alex')
''
> qstr(24)
''
> qstr(['alex'])
'0=alex'
> qstr({name: 'alex'})
'name=alex'
> qstr({name: null})
'name='
> qstr(null)
''
> qstr(undefined)
''

JSON.stringify accepts everything (sorta):

> jstr('alex')
'"alex"'
> jstr(24)
'24'
> jstr(['alex'])
'["alex"]'
> jstr({name: 'alex'})
'{"name":"alex"}'
> jstr(null)
'null'
> jstr(undefined)
undefined

Proposals:

  1. Just support their common subset. Keep the API simple. body validation wouldn't depend on which of the flags you're using. Their common subset would be (Array and Object).
  2. Stringify what the Node supports but don't stringify null or undefined, the user probably meant 'don't send a body'.
  3. Don't stringify null or undefined. Stringify what makes sense for each. Querystring that's Array and Object. (shh, don't worry about deep objects). For JSON that's Number, String, Array, and Object. I for one do use the plain values in my API. Example: add ten to a user's credits, or shift the position of an item in a list.

I vote 3 🙋.

Sidenote: wouldn't mind leaving this out of the milestone.

@sindresorhus
Copy link
Owner

I vote 1. I'm ok with also supporting an array, but querystring.stringify is IMHO too loose.

I for one do use the plain values.

Really? I've never seen that before.

@alextes
Copy link
Contributor

alextes commented May 6, 2017

Alright, I'll do 1. but when the people come knocking for us to be less opinionated and support what is valid JSON, I'm with them ✊.

@sindresorhus
Copy link
Owner

@alextes That's actually a good way to do development. Let's call it Complaint-Based-Development. It's so easy to try to solve every imaginary use-case and get bloated. Implementing the minimum and rather change later if many people complain means you only implement what most people need.

@alextes
Copy link
Contributor

alextes commented May 6, 2017

@sindresorhus Complaint-Based-Development 😁. Maybe call it Wish-Based-Development ✨ instead 🙏. Fully agree with all your points!

@ash0080
Copy link

ash0080 commented Nov 8, 2018

Not interested adding a temporary workaround when it's possible for you to stringify it yourself. We'll add support for passing object literal in body when json: true in the next major release.

@sindresorhus So how to make a request with json content but response is a html?

@szmarczak
Copy link
Collaborator

const instance = got.extend({
    hooks: {
        beforeRequest: [
            options => {
                options.body = JSON.stringify(options.body);
            }
        ]
});

instance(url, options);

This was referenced Dec 9, 2018
@alextes
Copy link
Contributor

alextes commented Dec 25, 2018

I'd go with something simpler 😊:

got(url, {
	headers: {
		'content-type': 'application/json'
	},
	body: JSON.stringify(body);
});

@szmarczak
Copy link
Collaborator

Custom instances are even simpler ;) No need to set headers every time or stringify the body :P

@alextes
Copy link
Contributor

alextes commented Dec 27, 2018

Alright, alright 😄 . Still like this version better in that case. No need to understand got's request lifecycle. Although, you got to hand it to you (pun intended?), not having to stringify the body is pretty cool 😎 .

const jsonGot = got.extend({
	headers: {
		'content-type': 'application/json'
	}
})
jsonGot({
    url,
    body: JSON.stringify(post)
})

@ash0080
Copy link

ash0080 commented Jan 15, 2019

Excuse me? what u discussed is requesting a json, but what I asked is post a json, responsed is html 😂

@szmarczak
Copy link
Collaborator

So how to make a request with json content but response is a html?

As far as I'm good with English, IOW you said to make a request which body is JSON, and the response body is HTML. So I think you're confused here :)

@alextes
Copy link
Contributor

alextes commented Jan 15, 2019

Ah, we should've been a bit clearer @ash0080 .

got(url, {
	method: 'POST',
	headers: {
		'content-type': 'application/json'
	},
	body: JSON.stringify(body);
});

However, got is quite smart. It understands that if you're adding a body, you probably meant to post, and will set the method to POST all by itself, unless explicitly instructed otherwise.

@ash0080
Copy link

ash0080 commented Jan 23, 2019

Ahhha~ Sorry, I am messed up. what we discussed is right.
Sorry for my chaotic mind 😂

@cocuba
Copy link

cocuba commented Jun 3, 2022

hi, i have this problem, what could be?

got(url, {
    method: "POST",
    headers: {
      Authorization: process.env.USER_TOKEN,
      "content-type": "application/json",
    },
    body: JSON.stringify({
      start: dateFrom.toISOString(),
      end: dateTo.toISOString(),
      office_ids: [officeId],
    }),
  }).json();

and obtained this response (in the image shows the body sended)
image

thanks in advance!

@alextes
Copy link
Contributor

alextes commented Jun 3, 2022

@cocuba this is a veeerry old issue. I suggest you create a new one 😉 .

To try and help you along a little. What it's saying is the response you got back is not a JSON body. Got tried to decode it but the first token, that is, the first character in the response body is not valid for a JSON body.

You can use .text instead of .json to try and find out what it is you are getting.

@cocuba
Copy link

cocuba commented Jun 3, 2022

@cocuba this is a veeerry old issue. I suggest you create a new one 😉 .

To try and help you along a little. What it's saying is the response you got back is not a JSON body. Got tried to decode it but the first token, that is, the first character in the response body is not valid for a JSON body.

You can use .text instead of .json to try and find out what it is you are getting.

ohhh!!! thanks. the response is the problem, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This change will extend Got features ✭ help wanted ✭
Projects
None yet
Development

No branches or pull requests