-
Notifications
You must be signed in to change notification settings - Fork 139
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
Configure http method used #3
Conversation
}; | ||
|
||
if (graphQLMethod.toLowerCase() === 'get') { | ||
query += "?query=" + graphQLParams.query; |
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.
Interesting, what is graphQLParams.query
?
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.
It's the GraphQL query (like { posts { id, title } }
). For a GET
request we can't send it in the request body like we do for POST
, it needs to be the query
param.
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.
Ohh right, graphQLParams
is just a POJO with query
and variables
properties. Derp, sorry spaced on that. Should get
requests also include query variables?
for example
query += "&variables=" + JSON.stringify(graphQLParams.variables)
Or, are they being sent to the server some other way?
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.
Oh, good point, we need to add the variables
there as well. What about something like this:
var queryParams = Object.keys(graphQLParams).map(function(key) {
return key + "=" + graphQLParams[key];
});
query += "?" + queryParams.join("&");
So we always send all the params in graphQLParams
, like we do for POST
requests.
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.
That seems good! Do you need JSON.stringify
? Otherwise, objects might be represented as [Object object]
(or whatever) 😬
Maybe also encodeURIComponent
?
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.
Unfortunately JSON.stringify
won't work as these values are already strings. It will try to "stringify" a string by escaping the quotes, which causes a parser error (stacktrace). I think we'd need a special handling to add objects in the url anyway.
encodeURIComponent
doesn't seem to make a difference, maybe fetch.js
is already taking care of that, but I can add it if you want.
I was curious to know why this Pull Request was closed without merging. We are using the I would be happy to help get this merged if it was a bandwidth issue, or is there something else going on that prevents this approach from working? |
With this change we should be able to define a
graphql_method
that will be used to send requests to the graph endpoint.Closes #2