-
Notifications
You must be signed in to change notification settings - Fork 423
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
feat: add integration of serde_json #975
base: master
Are you sure you want to change the base?
Conversation
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.
@chirino thanks for bringing this up! 🍻
Seems to be a nice implementation idea for a long-waited feature! But more polishing and work should be done to cover all the nececities.
I've corrected the codestyle a bit, linked the related issue/PRs to it and described the checklist of what is awaited to be done for this.
Also, it would be better to mark this PR as draft until it's not ready for review, so I don't review it accidentally!
Also what do you mean by tested as field of input/output type? Does that mean as field of a struct using the |
@chirino yes #[derive(GraphQLInputObject)]
struct InputObj {
val: serde_json::Value,
}
struct Human;
#[graphql_object]
impl Human {
fn output() -> serde_json::Value {
}
fn input(arg: serde_json::Value) -> bool {
}
fn input_object(obj: InputObj) -> bool {
}
} |
Sadly we can't do something as simple as:
Since we need a way to associate type info with the val field. I did included a test that shows how you can create a wrapper type around Do you have an example of how you test the subsctiption return type case? |
Does it worth to have such a wrapper predefined?
See |
…tic type info for serde_json::Value
I think the PR should have about the right stuff covered in the tests. It introduces a couple of new public types... and naming things is hard so let me know if you can think of better names for:
Also not sure where to document this in the book. |
@chirino go with the names you think are best. I'll bikeshed them in the final review if a better naming will come up in my mind.
I think it would be nice to create a separate page in advanced topics. |
Minor nit, but we probably want to match the integration crate name with the integration file name with the flag name. That is, |
…rde_json support. Also renamed json.rs to serde_json.rs.
renamed and added a section to the book. Is anything else needed? |
@chirino thanks! I'll look at it in few days. |
This is great work, thank you! ❤️ |
Just to keep everybody up-to-date: |
@tyranron thanks for the update. Do you have a branch of your work that I can peek at? Maybe I can find some time to help. |
@chirino unfortunately no, but thank you. You've already done so much 🙇♂️ I just need to somewhat extend that, fix some edge cases and polish. It will be harder to explain than do and finish. |
Wow, this PR has turned to be such a huge rabbit hole 😅 Sorry for moving it slowly, but I'd like to do it the proper way from the start, rather than having it half-baked or redoing in future. |
No rush. Glad my "first ever rust PR" is nudging this project even to become even better. |
@tyranron hey.. I know this is a complex PR and I'm a noob, but feel free to reach out to me if you need help with it. |
@tyranron are you still planning on working on this PR? Super excited to get |
@juancampa yes, of course. It's scheduled for 0.16.0 release. But it requires first the #1072 to be completed, as at the moment there is no way to combine dynamic GraphQL schema with a static one, due to |
@tyranron should I close this PR out? |
1 similar comment
@tyranron should I close this PR out? |
Resolves #957
Fixes #280, #504, #509
Superseeds #325
Implements the
GraphQLValue
trait forserde_json::Value
. It's type info is given using an SDL definition.Checklist
GraphQLType
andGraphQLValue
traitsGraphQLAsyncValue
traitIsOutputType
marker andIsInputType
markerStrem::Item
)async
context