Skip to content

Allow using #[derive(GraphQLQuery)] without depending on serde #487

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.

Sign up for GitHub

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

Merged
tomhoule merged 1 commit into from
Jan 22, 2025
Merged

Allow using #[derive(GraphQLQuery)] without depending on serde #487

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion examples/github/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ edition = "2018"
[dev-dependencies]
anyhow = "1.0"
graphql_client = { path = "../../graphql_client", features = ["reqwest-blocking"] }
serde = "^1.0"
reqwest = { version = "0.12", features = ["json", "blocking"] }
prettytable-rs = "^0.10.0"
clap = { version = "^3.0", features = ["derive"] }
Expand Down
1 change: 0 additions & 1 deletion examples/hasura/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ edition = "2018"
[dev-dependencies]
anyhow = "1.0"
graphql_client = { path = "../../graphql_client", features = ["reqwest-blocking"] }
serde = { version = "1.0", features = ["derive"] }
serde_json = "1.0"
reqwest = { version = "0.12", features = ["json", "blocking"] }
prettytable-rs = "0.10.0"
Expand Down
1 change: 0 additions & 1 deletion examples/web/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ crate-type = ["cdylib", "rlib"]
[dependencies]
graphql_client = { path = "../../graphql_client", features = ["reqwest"] }
wasm-bindgen = "^0.2"
serde = { version = "1.0.67", features = ["derive"] }
lazy_static = "1.0.1"
js-sys = "0.3.6"
wasm-bindgen-futures = "0.4.18"
Expand Down
6 changes: 6 additions & 0 deletions graphql_client/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,12 @@ pub struct Response<Data> {
pub extensions: Option<HashMap<String, serde_json::Value>>,
}

/// Hidden module for types used by the codegen crate.
#[doc(hidden)]
pub mod _private {
pub use ::serde;
}

Comment on lines +302 to +307
Copy link
Contributor

@caspermeijn caspermeijn Jul 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am conflicted on whether I like how this re-export works.

  • First of all, I don't think you have to hide it. I think explicitly depending on serde 1 is not a problem. But I can't find best practices to back up that statement.
  • Does this re-export belong here, or in graphql_query_derive as that crate defines the macro that uses it, or in graphql_query_codegen as that crate produces the generated code?

Copy link
Contributor Author

@swlynch99 swlynch99 Jul 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A hidden module is a common pattern for private types used in macros (see serde, for an example). Adding a new public export is a bigger change than having it as a private module, regardless of whether publically exporting serde is an issue or not. I have not seen a crate publically re-export serde, though.

For the second point, this export needs to be in graphql_client. That is the only crate that we know will be available to users of the library. Beyond that, you can't have non-proc-macro exports from a proc-macro crate which rules out exporting it from graphql_query_derive.

Copy link
Contributor

@caspermeijn caspermeijn Aug 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the second point, this export needs to be in graphql_client. That is the only crate that we know will be available to users of the library. Beyond that, you can't have non-proc-macro exports from a proc-macro crate which rules out exporting it from graphql_query_derive.

I didn't know about that rule for proc-macro exports, then It makes total sense to do it in graphql_client.

#[cfg(test)]
mod tests {
use super::*;
Expand Down
11 changes: 9 additions & 2 deletions graphql_client_codegen/src/codegen.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use crate::{
};
use heck::ToSnakeCase;
use proc_macro2::{Ident, Span, TokenStream};
use quote::quote;
use quote::{quote, ToTokens};
use selection::*;
use std::collections::BTreeMap;

Expand All @@ -21,6 +21,8 @@ pub(crate) fn response_for_query(
options: &GraphQLClientCodegenOptions,
query: BoundQuery<'_>,
) -> Result<TokenStream, GeneralError> {
let serde = options.serde_path();

let all_used_types = all_used_types(operation_id, &query);
let response_derives = render_derives(options.all_response_derives());
let variable_derives = render_derives(options.all_variable_derives());
Expand All @@ -43,7 +45,7 @@ pub(crate) fn response_for_query(
render_response_data_fields(operation_id, options, &query).render(&response_derives);

let q = quote! {
use serde::{Serialize, Deserialize};
use #serde::{Serialize, Deserialize};
use super::*;

#[allow(dead_code)]
Expand Down Expand Up @@ -77,9 +79,13 @@ fn generate_variables_struct(
options: &GraphQLClientCodegenOptions,
query: &BoundQuery<'_>,
) -> TokenStream {
let serde = options.serde_path();
let serde_path = serde.to_token_stream().to_string();

if operation_has_no_variables(operation_id, query.query) {
return quote!(
#variable_derives
#[serde(crate = #serde_path)]
pub struct Variables;
);
}
Expand Down Expand Up @@ -115,6 +121,7 @@ fn generate_variables_struct(

let variables_struct = quote!(
#variable_derives
#[serde(crate = #serde_path)]
pub struct Variables {
#(#variable_fields,)*
}
Expand Down
11 changes: 6 additions & 5 deletions graphql_client_codegen/src/codegen/enums.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ pub(super) fn generate_enum_definitions<'a, 'schema: 'a>(
options: &'a GraphQLClientCodegenOptions,
query: BoundQuery<'schema>,
) -> impl Iterator<Item = TokenStream> + 'a {
let serde = options.serde_path();
let traits = options
.all_response_derives()
.chain(options.all_variable_derives())
Expand Down Expand Up @@ -66,18 +67,18 @@ pub(super) fn generate_enum_definitions<'a, 'schema: 'a>(
Other(String),
}

impl ::serde::Serialize for #name {
fn serialize<S: serde::Serializer>(&self, ser: S) -> Result<S::Ok, S::Error> {
impl #serde::Serialize for #name {
fn serialize<S: #serde::Serializer>(&self, ser: S) -> Result<S::Ok, S::Error> {
ser.serialize_str(match *self {
#(#constructors => #variant_str,)*
#name::Other(ref s) => &s,
})
}
}

impl<'de> ::serde::Deserialize<'de> for #name {
fn deserialize<D: ::serde::Deserializer<'de>>(deserializer: D) -> Result<Self, D::Error> {
let s: String = ::serde::Deserialize::deserialize(deserializer)?;
impl<'de> #serde::Deserialize<'de> for #name {
fn deserialize<D: #serde::Deserializer<'de>>(deserializer: D) -> Result<Self, D::Error> {
let s: String = #serde::Deserialize::deserialize(deserializer)?;

match s.as_str() {
#(#variant_str => Ok(#constructors),)*
Expand Down
6 changes: 5 additions & 1 deletion graphql_client_codegen/src/codegen/inputs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use crate::{
};
use heck::{ToSnakeCase, ToUpperCamelCase};
use proc_macro2::{Ident, Span, TokenStream};
use quote::quote;
use quote::{quote, ToTokens};

pub(super) fn generate_input_object_definitions(
all_used_types: &UsedTypes,
Expand All @@ -33,6 +33,9 @@ fn generate_struct(
variable_derives: &impl quote::ToTokens,
query: &BoundQuery<'_>,
) -> TokenStream {
let serde = options.serde_path();
let serde_path = serde.to_token_stream().to_string();

let normalized_name = options.normalization().input_name(input.name.as_str());
let safe_name = keyword_replace(normalized_name);
let struct_name = Ident::new(safe_name.as_ref(), Span::call_site());
Expand Down Expand Up @@ -71,6 +74,7 @@ fn generate_struct(

quote! {
#variable_derives
#[serde(crate = #serde_path)]
pub struct #struct_name{
#(#fields,)*
}
Expand Down
8 changes: 6 additions & 2 deletions graphql_client_codegen/src/codegen/selection.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use crate::{
};
use heck::*;
use proc_macro2::{Ident, Span, TokenStream};
use quote::quote;
use quote::{quote, ToTokens};
use std::borrow::Cow;

pub(crate) fn render_response_data_fields<'a>(
Expand Down Expand Up @@ -433,7 +433,7 @@ impl ExpandedField<'_> {
(Some(msg), DeprecationStrategy::Warn) => {
let optional_msg = msg.map(|msg| quote!((note = #msg)));

Some(quote!(#[deprecated#optional_msg]))
Some(quote!(#[deprecated #optional_msg]))
}
(Some(_), DeprecationStrategy::Deny) => return None,
};
Expand Down Expand Up @@ -532,6 +532,9 @@ impl<'a> ExpandedSelection<'a> {
}

pub fn render(&self, response_derives: &impl quote::ToTokens) -> TokenStream {
let serde = self.options.serde_path();
let serde_path = serde.to_token_stream().to_string();

let mut items = Vec::with_capacity(self.types.len());

for (type_id, ty) in self.types() {
Expand Down Expand Up @@ -600,6 +603,7 @@ impl<'a> ExpandedSelection<'a> {

let tokens = quote! {
#response_derives
#[serde(crate = #serde_path)]
pub struct #struct_name {
#(#fields,)*
#on_field
Expand Down
13 changes: 13 additions & 0 deletions graphql_client_codegen/src/codegen_options.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ pub struct GraphQLClientCodegenOptions {
fragments_other_variant: bool,
/// Skip Serialization of None values.
skip_serializing_none: bool,
/// Path to the serde crate.
serde_path: syn::Path,
}

impl GraphQLClientCodegenOptions {
Expand All @@ -68,6 +70,7 @@ impl GraphQLClientCodegenOptions {
extern_enums: Default::default(),
fragments_other_variant: Default::default(),
skip_serializing_none: Default::default(),
serde_path: syn::parse_quote!(::serde),
Copy link
Contributor

@caspermeijn caspermeijn Jul 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that the generated code is already dependent on ::graphql_client, I don't think this has to be configurable. Do you have a usecase for having this configurable?

Copy link
Contributor Author

@swlynch99 swlynch99 Jul 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The graphql_client_codegen crate is used outside of the derive macro (see its reverse dependencies). Unilaterally changing the path used for serde imports would break users who are generating code for an older graphql_client version. Notably, it would change the output of the CLI.

It's been a while since I wrote the code here but I think that was the reason that I did it as an option here.

Copy link
Contributor

@caspermeijn caspermeijn Aug 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The graphql_client_codegen crate is used outside of the derive macro (see its reverse dependencies). Unilaterally changing the path used for serde imports would break users who are generating code for an older graphql_client version. Notably, it would change the output of the CLI.

You are right, this causes the least breaking change. The problem I see is that a config option is not good for discoverability. However, if you are using the codegen crate, then you are probably an advanced user. I think it makes sense to do it this way.

}
}

Expand Down Expand Up @@ -227,4 +230,14 @@ impl GraphQLClientCodegenOptions {
pub fn skip_serializing_none(&self) -> &bool {
&self.skip_serializing_none
}

/// Set the path to used to resolve serde traits.
pub fn set_serde_path(&mut self, path: syn::Path) {
self.serde_path = path;
}

/// Get a reference to the path used to resolve serde traits.
pub fn serde_path(&self) -> &syn::Path {
&self.serde_path
}
}
1 change: 1 addition & 0 deletions graphql_query_derive/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ fn build_graphql_client_derive_options(
options.set_struct_ident(input.ident.clone());
options.set_module_visibility(input.vis.clone());
options.set_operation_name(input.ident.to_string());
options.set_serde_path(syn::parse_quote!(graphql_client::_private::serde));

Ok(options)
}
Loading