Provide variables as a JSON Object instead of a String

[OlegIlyenko]

During one of the discussions with @stubailo and @lacker at https://github.com/graphql/graphql.github.io we decided to suggest this change (tried to find the original discussion, but looks like it's gone due to fair amount of rebasing).

Also current best practice documentation suggests that variables should be provided as a JSON object and not as a string (at the moment GraphiQL provides variables as a string with stringified JSON inside):

http://graphql.org/learn/serving-over-http/#post-request

[asiandrummer]

I think it'd be better to wrap JSON.parse in a try/catch block to avoid a console error:

let jsonVariables = null;
try {
  jsonVariables = JSON.parse(variables);
} catch (error) {
  jsonVariables = variables || null;
}

const fetch = fetcher({ ... });

What do you think?

[OlegIlyenko]

It's a good point. I guarded from an empty strings here, but it's possible that input is malformed. In this case it would be an error in the console and no request would be sent (a bit confusing to the user).

I agree, catching exception would be more robust error handling here. Though I would suggest to always set jsonVariables to null:

jsonVariables = null;

because of the 2 reasons:

• At this point we already know that JSON is either malformed or empty, so it doesn't make sense to send it to the server
• In a long term it would be nice to require server to support only JSON object for variables and not both: string and JSON object (which is the case at the moment - most server implementations support string variables only to be compatible with GraphiQL, for all other requests sources JSON object is expected)

WDYT?

[asiandrummer]

Sounds good. Your last point got me thinking - I think this might break any graphQLFetchers and/or server implementations if it was expecting a string for variables. For instance, a version of graphQLFetcher may serialize the variables string beforehand, and this change will supply an JSON object instead of a string, possibly resulting in breaking the app.

Having said this, I think it's probably okay to merge this and then note as a possible breaking change for next patch release.

[OlegIlyenko]

I definitely share the concern about backward compatibility. One possibility would be to add an extra step in the migration path and initially support a prop that configures variables format (string/json). This will make the migration a little bit more graceful, but still it would be a breaking change. Though I wonder how many servers do not support JSON object variables and support only string.

[asiandrummer]

I'd be okay with a breaking change and communicating this thoroughly with the next release. Another prop sounds a bit over-complicated.

[asiandrummer]

From my last comment:

jsonVariables = null;

This sounds good as well!

[OlegIlyenko]

done :)

[leebyron]

Are we sure we want to do this? Either way it ends up being sent as a string to the server since TCP/HTTP doesn't know anything about JSON - and servers still need to be able to handle malformed input with appropriate error. Right now you can try to send invalid JSON via the fetcher and you'll get back whatever the server thinks is an appropriate error.

This essentially just makes it the requirement of the "fetcher" to then convert this JSON object back into a string before constructing a URL or a POST request.

This would also be a breaking change, so we should be really sure that this is adding value if we do it. It will result in additional work for many people who use GraphiQL.

[leebyron]

(To be clear - I'm not suggesting we not do it, I just want to ensure we have a strong argument for it)

[OlegIlyenko]

I agree, thanks for raising this point, Lee. In fact I pointed out that we need to consider the way GraphiQL handles variables when we were discussing this part of the documentation: http://graphql.org/learn/serving-over-http/#post-request. But general consensus was that it would be batter to provide variables as a JSON object. This is where this PR came from.

My personal arguments in favor of this change:

• From what I can tell, for most uses I talked to, it comes as a surprise and annoyance that GraphiQL provides variables as a string (myself included). So the servers are kind of forced to handle both cases: string for GraphiQL and JSON object for all other clients. I guess it's more about human factor and learning experience rather than anything else. I noticed that for most people it feels very unnatural to parse a JSON string (with stringified JSON inside) contained within a JSON document.
• After implementing GraphQL servers using other formats and protocols (just a raw TCP + binary data formats like MessagePack and Amazon Ion), I getting more convinced that it is very natural/intuitive and convenient to have variables in the same format as a request message. In case of GraphiQL it's a JSON, in case of my TCP example servers is's some binary format.
• The best practice documentation explicitly states that this is the recommended way of handling variables.
• Most of the client applications will actually define variables in javascript code as a JSON objects. So I would consider the GraphiQL use-case to be an exception since it gets variables from a codemirror as a string. For me it's had to find a convincing argument to force all GraphQL servers to support this format just for this very unique use-case.

I would also appreciate feedback from @stubailo and @lacker.

[leebyron]

The best practice documentation explicitly states that this is the recommended way of handling variables.

Sure, but we wrote that, so tautology ;)

The rest of your points are really good though, thanks for articulating all of that. Certainly if we're going to suggest that the query and variables each be keys in a JSON-formatted payload that a json string within a json object is a little strange.

[leebyron]

One last thing that I'm a little nervous about is the behavior when JSON is malformed. I think we should ensure that there's really clear error messaging behavior so the dev understands what went wrong and how to fix it.

Certainly throwing errors into the console is useless, so I'm glad we avoided that route. I'm concerned that eating the JSON error and just setting variables to null also isn't a great behavior - that means if I typo in my JSON that the error will be a failure to provide variables for required variables or just unexpected behavior for optional variables. I think it will not be obvious for the dev to go look for a JSON formatting error in the variables pane to fix unexpected behavior - they might instead assume something is wrong with the server.

Right now when you send invalid JSON (to a graphql-js server) you'll get the error:

{
  "errors": [
    {
      "message": "Variables are invalid JSON."
    }
  ]
}

We should probably either not let you send a request at all if the variables JSON is malformed, and we should probably report back into the response pane exactly why.

[OlegIlyenko]

Sure, but we wrote that, so tautology ;)

true :) Now that I read it again, the wording does not communicate my original point :) What I meant with this point is that now we officially provide a best practice for this. The set of standard tools should be consequent and also follow these best practices otherwise it can cause confusion. I just thinking about new users who followed best practices and implemented a GraphQL server. Now with a lot of excitement they would like to experiment with GraphiQL and see it working for their very own GraphQL API just to discover that every request is failing with some json parsing exception on the server. I guess it will take a while to figure out not only why exactly it is failing (it's the easier part) but also why GraphiQL sends a string instead of recommended JSON object. As far as I'm aware it's not mentioned or explained anywhere (even in the GraphiQL readme).

We should probably either not let you send a request at all if the variables JSON is malformed, and we should probably report back into the response pane exactly why.

I totally agree! I must admit, I haven't thought through the error reporting in the original PR. I guess I learned good lesson here :) I will prepare another PR to address this issue. Showing a feedback in the response pane without sending the request sounds like a good solution to me.