Allow using #[derive(GraphQLQuery)] without depending on serde
#487
Conversation
caspermeijn
left a comment
caspermeijn
left a comment
There was a problem hiding this comment.
I am not the maintainer. I had this same issue, so I want to help out with a review.
I think a re-export of serde is the way to go. That way the users of graphql_client don't have to be aware of that dependency.
| /// Hidden module for types used by the codegen crate. | ||
| #[doc(hidden)] | ||
| pub mod _private { | ||
| pub use ::serde; | ||
| } | ||
|
|
There was a problem hiding this comment.
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_deriveas that crate defines the macro that uses it, or ingraphql_query_codegenas that crate produces the generated code?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 fromgraphql_query_derive.
I didn't know about that rule for proc-macro exports, then It makes total sense to do it in graphql_client.
| extern_enums: Default::default(), | ||
| fragments_other_variant: Default::default(), | ||
| skip_serializing_none: Default::default(), | ||
| serde_path: syn::parse_quote!(::serde), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The
graphql_client_codegencrate 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 oldergraphql_clientversion. 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.
tomhoule
left a comment
tomhoule
left a comment
There was a problem hiding this comment.
Looks great, thanks for the PR!
|
The branch needs a rebase. I can take care of it next time I spend some time on graphql-client (ETA: unknown). |
This PR makes the serde derives emitted by `#[derive(GraphQLQuery)]` use
a private re-export of the serde crate instead of relying on users of
this crate having added serde to their `Cargo.toml`.
Serde allows you to add an annotation
#[serde(crate = "<path to crate>")]
for exactly this reason so most of this PR is just plumbing the correct
path through the codegen crate to the correct annotation.
Details
-------
- There is a new `#[doc(hidden)] mod _private` declaration in the
`graphql_client` crate. All re-exports (which is just serde) have been
placed there.
- I have added a new `serde_path` field to `GraphQLCodegenOptions` which
defaults to `::serde`. This means that the code generated by the CLI
should remain effectively unchanged.
- The rest is just applying `#[serde(crate = ...)]` annotations where
appropriate.
Fixes graphql-rust#427
swlynch99
force-pushed
the
avoid-user-serde
branch
from
830d890 to
a4389c8
Compare
|
I have rebased on latest main. I suspect I'll still need manual approval to get CI to run on the rebased commit. |
3 participants
This PR makes the serde derives emitted by
#[derive(GraphQLQuery)]use a private re-export of the serde crate instead of relying on users of this crate having added serde to theirCargo.toml.Serde allows you to add an annotation
for exactly this reason so most of this PR is just plumbing the correct path through the codegen crate to the correct annotation.
Details
#[doc(hidden)] mod _privatedeclaration in thegraphql_clientcrate. All re-exports (which is just serde) have been placed there.serde_pathfield toGraphQLCodegenOptionswhich defaults to::serde. This means that the code generated by the CLI should remain effectively unchanged.#[serde(crate = ...)]annotations where appropriate.Fixes #427