-
-
Notifications
You must be signed in to change notification settings - Fork 320
Deprecate most GraphService methods and converge on two standard methods #1051
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
Conversation
Should it be also be possible to provide variables via a dictionary? |
Oh yeah, I like that more @clement911! So maybe we change the GraphRequest class to look like this: public class GraphRequest
{
public string Query { get; set; }
public Dictionary<string, object> Variables { get; set; }
// Convenience methods for adding/setting variables?
public GraphRequest AddVariable(string key, object value)
{
Variables.Add(key, value);
return this;
}
public GraphRequest SetVariable(string key, object value)
{
Variables.Set(key, value);
return this;
}
} Then you could just pass in an entire dictionary if you've already got one, use the convenience methods, or access the dictionary object directly. |
I'm not sure that the |
Agreed, I was thinking about that too and it doesn’t sound that useful. I’m going to drop that idea, but I did make the change to switch the Variables type to a dictionary.—Joshua HarmsOn Apr 7, 2024, at 17:19, Clement Gutel ***@***.***> wrote:
I'm not sure that the AddVariable / SetVariable methods really are needed.
The user can simply use the Variables dictionary to manage variables, right?
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>
|
2f61904
to
05fa543
Compare
02ea7b9
to
5647733
Compare
18ca45f
to
8e15fe8
Compare
Remaining tasks:
|
e6492c3
to
4daf9f1
Compare
05eb6ac
to
22d58a9
Compare
22d58a9
to
82af200
Compare
d7f7a5c
to
bc127ce
Compare
@nozzlegear I know that you are actively working on this and wondered if you saw this: https://shopify.dev/changelog/content-type-application-graphql-is-deprecated @matnun-br FYI |
@clement911 Oh yes, I did see that! Thanks for the reminder, I want to incorporate that. |
106d636
to
a9a161f
Compare
…to protected This change was done largely to facilitate easier direct testing of the ParseGraphExtensions method (without the convoluted step of sending requests through other unrelated methods first), but it makes sense to allow inheriting classes to access these two utility functions.
Just a nice change to have in unit tests for the equality checks.
…e)` and covering unit tests
…lable `T` The System.Text.Json deserializer will only return null in very limited edge-cases, chiefly when the json string is the json literal `null` or the deserialized type is a nullable primitive (e.g. `int?`, `bool?`) and the json structure doesn't match. Even an empty curly brace will not trip the json literal `null` case. However, the GraphService itself has multiple checks in place to ensure that the json string is not null, an empty curly brace or even a mismatched type – it expects `object` and will only operate on `object`, and it will throw an exception if it doesn't find and object. Given this, we can make the assumption that the deserialization will succeed and the object will not be null.
…y throw when GraphResult.Json is not a JsonElement
…null/empty but message is not
7431b9a
to
28ada09
Compare
Merged! 🎉 I'll check the tests, draft a new Github/Nuget release and begin updating the documentation tomorrow afternoon. |
I'm sure I'm the last visual basic programmer on the planet, but now this class is crashing as VB can't handle there being a query property and a Query property. |
@dg107 Oh, I didn't know that. Thanks for the heads up! I mostly use F# in my personal projects so I know the pain of devs assuming if something works in C# it must work in everything else. 😜 I'll do some digging and see if I can't get this fixed somehow. |
Hey @dg107 (and @CopilotDaniel) – I've created a separate issue for the Visual Basic problem. I'm not sure if you've seen it, can you check the solution I've proposed there and let me know if it'll work for you? Here's the link: #1144 |
This change deprecates most GraphService.PostAsync and GraphService.SendAsync methods, of which there were at least six total. It introduces two new GraphService.PostAsync methods to replace them all, which standardize the usage of the
GraphRequest
class and serializing all requests to json.Previously, some of the PostAsync/SendAsync methods would serialize the request to json, some wouldn't. Some would send the variables, some wouldn't. Some would let you set your own json wherein you could apply the variables yourself, but then they'd send the
application/graphql
content-type, which made Shopify ignore any variables sent.That one has personally bit me each time I've tried to use the GraphService recently; it's been difficult to figure out which method should be used on the service when I want to send variables, without looking at the source code for ShopifySharp.
After this change, the new method for sending Graph requests using the GraphService going forward would look like this:
I don't want anyone to lose the ability to control how variables are serialized or deserialized though, so I've added an
IGraphSerializer
interface that can be passed to the GraphService constructor.I'm not 100% sure this is the exact interface I'll use for this, but it's something along the lines of what I'm thinking. Whether we use this interface or do something else for serialization/deserialization in the GraphService, I want it to play nicely with the factory and DI package.